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 39ACB3858D1E for ; Mon, 9 Oct 2023 09:59:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 39ACB3858D1E 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=1696845597; 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=upTJZH25QvCvEyFsDUSG3YFWqd32CK43O6TgLpdRbEU=; b=fsjG2dA6naNiItEV5mUjnlZWnINqRP+EyI0U3wISWL18kgf9wPHlAihsgmTCP0DtInzWpa asURyqRddNqwQKj0b4SmAQUQSMWOJnh/6TXVTkf/lgJerfgcNsnA/W44IP/xOvLTgOr8Am YgrcZuSpLjHaQqlCoK0+uDGNszmrJYY= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-479-RIhC6-SGMi-P_2dauko0EA-1; Mon, 09 Oct 2023 05:59:55 -0400 X-MC-Unique: RIhC6-SGMi-P_2dauko0EA-1 Received: by mail-qv1-f72.google.com with SMTP id 6a1803df08f44-66216e7385fso54549706d6.3 for ; Mon, 09 Oct 2023 02:59:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696845594; x=1697450394; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=upTJZH25QvCvEyFsDUSG3YFWqd32CK43O6TgLpdRbEU=; b=XNNfNkWCRHXknVuRcwyEpFsLxGs6PpOBbBXafFWuUlWIB/HaAKr0BMM+Ilbl3pcPRp 1V+kla9I4mTPSXajdiFDnUCdQ4EEXwIeSzOguj7fvISbd+d6tWXrhCYRY/pRkCRvfNoO cZuChsZt7ZZWPqUDa1kQqBmsQrnvkQrURrvplx3s0jjRNrrowcWK3EspyP9g0Rw/SIv3 apmcA2o/LmC4HVBvywlzRahSDBCdUYCyGf7uDfRhlstvxq6nlXFpAtd6IsSoYMkiUl2M cYShfccld7cKbdgJOFqWR2/Dzfw4PAr2Mypq1HAu9ZaGzfMFv5mldiCmRT2AQ5bXm7eO tJ+g== X-Gm-Message-State: AOJu0Yz/ZqzvenMxnC397++Ktwjj6bE9zNJujWQETfNgO3sQe8UWxww/ o9GduZ52lmrCbfUYih2q6i6mUEsC7ZvXCAm2c52YJQe91wvtR/M+9SGBDVU5YP+wam5TDYyvNip XG3skRZX927Yni4knHPDfTBcumSNGTQ== X-Received: by 2002:a05:6214:4188:b0:655:d7b5:4728 with SMTP id ld8-20020a056214418800b00655d7b54728mr13727598qvb.54.1696845594296; Mon, 09 Oct 2023 02:59:54 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHvHhwWNAhe7F25xoEJasGJCpPu95H/6p6jhi8O/8s6PHeHdIIlJLiHq1Ta9OMyscwK5Vuudw== X-Received: by 2002:a05:6214:4188:b0:655:d7b5:4728 with SMTP id ld8-20020a056214418800b00655d7b54728mr13727587qvb.54.1696845593931; Mon, 09 Oct 2023 02:59:53 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-227-180.bb.vodafone.cz. [94.112.227.180]) by smtp.gmail.com with ESMTPSA id q8-20020a0cf5c8000000b0065d05c8bb5dsm3713878qvm.64.2023.10.09.02.59.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 09 Oct 2023 02:59:53 -0700 (PDT) Message-ID: Date: Mon, 9 Oct 2023 11:59:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH v5 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS To: Kevin Buettner Cc: gdb-patches@sourceware.org, eliz@gnu.org, Pedro Alves References: <20231005113533.86112-2-blarsen@redhat.com> <20231005113533.86112-3-blarsen@redhat.com> <20231005105530.63d11345@f37-zws-nv> From: Guinevere Larsen In-Reply-To: <20231005105530.63d11345@f37-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: 7bit X-Spam-Status: No, score=-5.1 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,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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: Top posting to ensure that both Eli and Pedro check your response. It is opposite to what was discussed on Cauldron, and I would like this worked out before sending a new version. On 05/10/2023 19:55, Kevin Buettner wrote: > On Thu, 5 Oct 2023 13:35:34 +0200 > Guinevere Larsen wrote: > >> In the GDB BoF in 2023's GNU tools cauldron it was discussed and agreed >> that Acked-by is already in use to represent partial approvals for >> projects like the Linux Kernel and QEMU, so it makes sense to use it >> similarly on this project. > [...] >> + - Acked-By: >> + >> + Used by a responsible or global maintainer when the patch touches multiple >> + areas of GDB, and the maintainer in question is only approving some of >> + those areas. When using this tag, add the area(s) at the end of the text. >> + This tag is also often described as "partial approval" >> + Usage: "Acked-By: Your Name (area)" > To indicate partial approval or partial review or even partial testing, > I recommend that we simply add "(area)" after the tag. > > E.g. if a reviewer has partially reviewed a patch and finds the parts > that change the gdbserver portions of it to be acceptable, they might > say: > > Reviewed-by: Random J Developer (gdbserver) > > The same could be done with the "Approved-by" tag. If this is done, > then multiple partial approvals may be needed. > > I think that an "Acked-by" tag could be useful when someone else has > done a detailed analysis and an area or global maintainer wishes to > indicate that they've read or skimmed the review, but have not done > any detailed analysis themselves. It might also be used to indicate > agreement with direction of the patch after reading the commit log. > But I don't think that it should be considered as strong as > "Reviewed-by" and definitely not as strong as "Approved-by". > > I reached those conclusions after looking at how two other projects, > the git and kernel projects, use "Acked-by" and "Reviewed-by". The > links that I looked at are: > > https://git-scm.com/docs/SubmittingPatches > https://docs.kernel.org/process/submitting-patches.html > > The git project has this to say about "Acked-by": > > Acked-by: says that the person who is more familiar with the area > the patch attempts to modify liked the patch. > > For the git project, "Reviewed-by" is much stronger: > > Reviewed-by: unlike the other tags, can only be offered by the > reviewers themselves when they are completely satisfied with the > patch after a detailed analysis. > > Here's how the kernel project uses Acked-by: > > Aked-by: is often used by the maintainer of the affected code > when that maintainer neither contributed to nor forwarded the > patch. > > Acked-by: is not as formal as Signed-off-by:. It is a record > that the acker has at least reviewed the patch and has indicated > acceptance. Hence patch mergers will sometimes manually convert > an acker's "yep, looks good to me" into an Acked-by: (but note > that it is usually better to ask for an explicit ack). > > Acked-by: does not necessarily indicate acknowledgement of the > entire patch. For example, if a patch affects multiple subsystems > and has an Acked-by: from one subsystem maintainer then this > usually indicates acknowledgement of just the part which affects > that maintainer's code. Judgement should be used here. When in > doubt people should refer to the original discussion in the > mailing list archives. > > However, for the kernel project, "Acked-by" is definitely not as > strong as "Reviewed-by": > > Reviewed-by:, instead, indicates that the patch has been reviewed > and found acceptable according to the Reviewer's Statement: > > Reviewer's statement of oversight > > By offering my Reviewed-by: tag, I state that: > > a. I have carried out a technical review of this patch to > evaluate its appropriateness and readiness for inclusion > into the mainline kernel. > > b. Any problems, concerns, or questions relating to the patch > have been communicated back to the submitter. I am > satisfied with the submitter's response to my comments. > > c. While there may be things that could be improved with this > submission, I believe that it is, at this time, (1) a > worthwhile modification to the kernel, and (2) free of > known issues which would argue against its inclusion. > > d. While I have reviewed the patch and believe it to be > sound, I do not (unless explicitly stated elsewhere) make > any warranties or guarantees that it will achieve its > stated purpose or function properly in any given situation. > > A Reviewed-by tag is a statement of opinion that the patch is an > appropriate modification of the kernel without any remaining > serious technical issues. Any interested reviewer (who has done > the work) can offer a Reviewed-by tag for a patch. This tag > serves to give credit to reviewers and to inform maintainers of > the degree of review which has been done on the patch. > Reviewed-by: tags, when supplied by reviewers known to understand > the subject area and to perform thorough reviews, will normally > increase the likelihood of your patch getting into the kernel. I understand what you mean, and I see where I may have misunderstood when I said that "Acked-By" worked as partial review in QEMU. I'm fine with this being the default if everyone agrees with it. To summarize informally, the tags would be like this: * Acked-By: A maintainer of a certain area looked at the patch description and is fine with its direction. this says nothing about the quality of the code. May be restricted to some areas of the code * Reviewed-By: A contributor has looked at the code and thinks it is good, but is not approving it for any reason. May be restricted to some areas of the code. * Approved-By: A maintainer has looked at the code and thinks that it is ready for upstreaming. May be restricted to some areas, and may be conditional on receiving a review or ack for some area(s). > For the GDB project, I have been thinking of "Approved-by" as meaning > "Reviewed-by"-with-approval. But I don't think that should > necessarily be the case. I think it makes sense for a global > maintainer to see that there are "Reviewed-by" and possibly "Acked-by" > tags and then make a decision to approve the patch (giving it an > "Approved-by" tag) without necessarily having done a detailed analysis > (i.e. review) themselves. > This makes sense, but it feels more like some informal thing to be decided between maintainers. -- Cheers, Guinevere Larsen She/Her/Hers