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.129.124]) by sourceware.org (Postfix) with ESMTPS id F27443854153 for ; Tue, 13 Jun 2023 07:44:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F27443854153 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=1686642242; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CZ1EMTDmGScq95nl23581kmqmJz29nYpu6qMwfDy89I=; b=ANtJumIpO8zf0O8IQCxhlYQYL94NdvvtgqXAyu7KX9yPjh0lefx4tQvCtrFFZ7wHHghdax oe4YflxkNmluHqH1B+RCrCw6tAq3dQyO4arpKM6f4QOOnQ9pMQvWas8CMePRzh/ZFDOpcF GYKUB9SKHAcxSuOSvhpPhO5HUNW64jk= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-355-FSCIDTaIO-aLx_bfAQS3pg-1; Tue, 13 Jun 2023 03:44:01 -0400 X-MC-Unique: FSCIDTaIO-aLx_bfAQS3pg-1 Received: by mail-ed1-f71.google.com with SMTP id 4fb4d7f45d1cf-51878f88529so478548a12.1 for ; Tue, 13 Jun 2023 00:44:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686642240; x=1689234240; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=CZ1EMTDmGScq95nl23581kmqmJz29nYpu6qMwfDy89I=; b=g84c/MRr61bYK9nVVcHuUnouAknsjGgelp1+QuIfVgKW12g4sFegBewdc3l/8+EHdD n+l0h1bJLleMwQhe08j6TDBAdf7rB5I4Ef5/HuDOg5Q26Dzk6VE+c3ecQmXkS2TpzyoJ 3nSYQSlVdyteTgC6zrq4Xg4fLw1ao8+Q+YGxl1fSOlksRJqzOxKpVUYqQnpry2rhcBuS tEYevxH1/6t9u1gWlDdkuU7d1xduvvMjbtJNY9Pvt2yuG8T5M1kFTR6FAXejh+Nl+Chk nlLl4P1HuiOqrF64cA+jlhoPbuVhQIpzqdutZi6ECb2KYB4Liz/ZvB8gzAt8OzogB2eP MN8w== X-Gm-Message-State: AC+VfDy86URsqEf+gs1PtUf/n6xPdN1xS2091So7ABrxSgWIv5/l+UbD ZD+pq8IWp38Sdw8bQHuOdrhgCEaUbUqQ1xgCUKI/OC02+gDMcP5213v1taLBu4Zscou/5ugUDco Gvd75wm7JoHBCubmVXmQObQ== X-Received: by 2002:a05:6402:34c9:b0:4ea:a9b0:a518 with SMTP id w9-20020a05640234c900b004eaa9b0a518mr7977338edc.17.1686642240139; Tue, 13 Jun 2023 00:44:00 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4wMjqnzRpcaJQ2yxTbVt9XElfumPKic/pw3HjCzNA5UrjJEGVlvGdBXwbSmKrNSAy/oiMlHQ== X-Received: by 2002:a05:6402:34c9:b0:4ea:a9b0:a518 with SMTP id w9-20020a05640234c900b004eaa9b0a518mr7977318edc.17.1686642239759; Tue, 13 Jun 2023 00:43:59 -0700 (PDT) Received: from [10.43.2.100] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id g25-20020a50ee19000000b0050bfeb15049sm5907975eds.60.2023.06.13.00.43.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 13 Jun 2023 00:43:59 -0700 (PDT) Message-ID: <284dc0ec-737b-4234-3a68-72b3de6c5a7a@redhat.com> Date: Tue, 13 Jun 2023 09:43:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v2 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS To: Kevin Buettner 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 References: <20230612100138.912813-1-blarsen@redhat.com> <20230612100138.912813-2-blarsen@redhat.com> <20230612165944.5bf15698@f38-zws-nv> From: Bruno Larsen In-Reply-To: <20230612165944.5bf15698@f38-zws-nv> 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: 8bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: On 13/06/2023 01:59, Kevin Buettner wrote: > Hi, > > Thanks for taking this on. See my thoughts inline below... > > On Mon, 12 Jun 2023 12:01:40 +0200 > Bruno Larsen wrote: > >> The project has been using Tested-By (tb), Reviewed-By (rb) and >> Approved-By (ab) for some time, but there has been no information to be >> found in the actual repository. This commit changes that by adding >> information about all git trailers to the MAINTAINERS file, so that it > If you wish to document *all* git trailers, then 'Bug:' and > 'Co-Authored-By:' should be documented too. Good point, I will add them to v3. I will just wait a bit for other people's input before sending a v3. >> can be easily double-checked. >> >> The upstream discussion also brought up the use of Acked-by, which is >> better defined in this commit. >> --- >> gdb/MAINTAINERS | 56 ++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 48 insertions(+), 8 deletions(-) >> >> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS >> index 175595d5f17..22f7ecbd471 100644 >> --- a/gdb/MAINTAINERS >> +++ b/gdb/MAINTAINERS >> @@ -43,14 +43,9 @@ patch without review from another maintainer. This especially includes >> patches which change internal interfaces (e.g. global functions, data >> structures) or external interfaces (e.g. user, remote, MI, et cetera). >> >> -The term "review" is used in this file to describe several kinds of feedback >> -from a maintainer: approval, rejection, and requests for changes or >> -clarification with the intention of approving a revised version. Review is >> -a privilege and/or responsibility of various positions among the GDB >> -Maintainers. Of course, anyone - whether they hold a position but not the >> -relevant one for a particular patch, or are just following along on the >> -mailing lists for fun, or anything in between - may suggest changes or >> -ask questions about a patch! >> +The word "contributor" is used in this document to refer to any GDB >> +developer listed above as well as folks who may have suggested some patches >> +but aren't part of one of those categories for any reason. >> >> There's also a couple of other people who play special roles in the GDB >> community, separately from the patch process: >> @@ -78,6 +73,51 @@ consensus among the global maintainers and any other involved parties. >> In cases where consensus can not be reached, the global maintainers may >> ask the official FSF-appointed GDB maintainers for a final decision. >> >> +The term "review" is used in this file to describe several kinds of feedback >> +from a maintainer: approval, rejection, and requests for changes or >> +clarification with the intention of approving a revised version. Approval is > In the line above, you've replaced the word "Review" with "Approval". > I think I like "Review" better. (But there may be an even better word > or phrase to use here.) I replaced it on purpose, because the full original wording is "Review is a privilege and/or responsibility (...) [of] GDB maintainers." This seems to imply that a new contributor may not review a patch if they aren't a maintainer yet, when the feeling I had of the patch and community as a whole was that anyone can review, what is a privilege that needs to be earned is approving patches.  If you think my reasoning is wrong, do let me know! > >> +a privilege and/or responsibility of various positions among the GDB >> +Maintainers. Of course, anyone - whether they hold a position but not the >> +relevant one for a particular patch, or are just following along on the >> +mailing lists for fun, or anything in between - may suggest changes, >> +ask questions about a patch or say if they believe a patch is fit for >> +upstreaming! >> + >> +To ensure that patches are only pushed when approved, and to thank the >> +contributors who take the time to review incoming patches, the GDB project >> +uses trailers to identify who contributed and how. All patches pushed >> +upstream should have at least one Approved-By patches (with the exception of >> +obvious patches, see below). The trailers (or tags) currently in use are: >> + >> + - Acked-By: >> + >> + Used when a contributor has taken a quick glance at a patch and agrees >> + with the direction outlined in the commit message, but hasn't evaluated >> + the code for correctness or regressions. >> + >> + - Tested-by: >> + >> + Used when a contributor does not want to comment on the quality >> + of the code in the patch, but has tested and sees no regressions on their >> + hardware. > For "Tested-by", I suggest this instead: > > Used when a contributor has tested the patch and finds that it > fixes the claimed problem. It may also be used to indicate that > the contributor has performed regression testing. By itself, this > tag says nothing about the quality of the fix implemented by the > patch. I like this better! I will use this on v3. > >> + >> + - Reviewed-by: >> + >> + Used when a contributors have looked at code and agrees with the changes, > s/contributors have/contributor has/ > s/code/the code/ oops, sorry, will change. > >> + but either don't have the authority or don't feel comfortable approving > s/don't/doesn't/ ?? ah, yes, leftover from a previous draft. > >> + the patch (usually due to unfamiliarity with the relevant part of the code). > I think I'd remove the parenthetical remark. There could be a variety of > reasons for feeling uncomfortable with outright approving the change. > One of the reasons that I sometimes don't approve a patch with which I > agree is that I want to give other maintainers a chance to look at it > first, especially when I know that they were involved with review of > earlier iterations of the patch. That's fair. I guess I don't need to give hints to the maintainers themselves here. > > I sometimes ask that a contributor wait a few days or a week for > others to comment prior to pushing it. I'm not sure how that would work > when using these tags. Could I give conditional 'Approved-by:' tag? Maybe > something like this?: > > Approved-by: (name & email here) > (But please wait a week for others to comment first. If there are any > objections or concerns, please address those first.) This is how I would recommend you go about things, yes. With the rb tag, I would have to wait an unknown amount of time before pushing, until some other maintainer gives me the ab tag or until you decide to look at a patch you've already reviewed, which is extra work for you. Conditional approvals, on the other hand, are already how we deal with partial approval. >> + >> + - Approved-by: >> + >> + Used by responsible maintainers or global maintainers when >> + a patch is ready to be upstreamed. Some patches may touch multiple areas >> + and require multiple approvals before landing (such as a maintainer only >> + approving documentation), it is up to the maintainer giving the approval tag >> + to make it clear when that a tag is not sufficient. >> + Responsible, Global and Official FSF-appointed maintainers may approve their >> + own patches, but it is recommended that they seek external approval before >> + doing so. >> + > In the above paragraph (and perhaps in a few other placess too), it > seems that the formatting needs some adjustment. (Some lines are > quite short while others seem a bit long.) Ah... the wonders of multiple drafts in an editor that doesn't format automatically, hahaha. Thanks for pointing this out! -- Cheers, Bruno > >> >> The Obvious Fix Rule >> -------------------- >> -- >> 2.40.1 >>