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 F1C2E3858CDB for ; Thu, 5 Oct 2023 17:55:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F1C2E3858CDB 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=1696528535; 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=WrQhuGxp3bJhIKjuxXrHYCZ5o95+Z8BdXF9df2iNYLk=; b=SXeytlUFNFKmSoaTI/J+OT6ECk6euvaboTfCJKQyBY46p/uJxFdUbP2bIGy408zMMIXo7+ mymdEUQMNLTC+z5AY1DJE6FHfuiHmD5nw0cEJRxl/HXFd6MJ3RnUkvCUCNsYKUx/x81uJZ y0EIBMo49OgCBAavZLWUbFL0VrPGu6U= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-558-yvs6hlaaMS-Fw4spIjA-Ww-1; Thu, 05 Oct 2023 13:55:32 -0400 X-MC-Unique: yvs6hlaaMS-Fw4spIjA-Ww-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 690513826D34; Thu, 5 Oct 2023 17:55:32 +0000 (UTC) Received: from f37-zws-nv (unknown [10.22.32.95]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F3CA1492B05; Thu, 5 Oct 2023 17:55:31 +0000 (UTC) Date: Thu, 5 Oct 2023 10:55:30 -0700 From: Kevin Buettner To: Guinevere Larsen Cc: gdb-patches@sourceware.org, eliz@gnu.org Subject: Re: [PATCH v5 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS Message-ID: <20231005105530.63d11345@f37-zws-nv> In-Reply-To: <20231005113533.86112-3-blarsen@redhat.com> References: <20231005113533.86112-2-blarsen@redhat.com> <20231005113533.86112-3-blarsen@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,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: 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. 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. Kevin