From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id D67C8385842D for ; Fri, 14 Jul 2023 05:50:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D67C8385842D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gnu.org Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qKBgx-0000gN-OI; Fri, 14 Jul 2023 01:50:11 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=uRRERaoWGN9Rpr4M+JM7/VUp8ttSgBLTuEtCRJLIHrk=; b=UnvbfNtgcDdE IMMe/3EhROjsiXaJTFleZ2lnkCIktHViSlDjllns/HfRKpBUwtBnrN9dxq4icqbO2q2qCe3RnTw2U 65lG4yY1Jz2N2vFRTQDBczkncFgmsTkEu6wdR4F/ayOX83cUEEwTqxz/UQXMIyhAz3QIWmDgLgS+S BiOVFNos89JU1jcbF5HNLK8Sd2bEHDEWzBs3EY4k6hhw5QvnFGfZD7Ava4qGgzTom04NyWuY3DJrZ G8copwmsdxXXBZo5BP7XNq+Ejla7pGh+myJmoio95Z/iewilxR+Y4JKKIcfUEv7gCfQsjiuQWFgZt FtGwj98UhaBiVHcE61KUEA==; Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qKBgw-0000Wg-W0; Fri, 14 Jul 2023 01:50:11 -0400 Date: Fri, 14 Jul 2023 08:50:28 +0300 Message-Id: <83h6q73uu3.fsf@gnu.org> From: Eli Zaretskii To: Kevin Buettner Cc: blarsen@redhat.com, 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 In-Reply-To: <20230713145047.358e2c4a@f37-zws-nv> (message from Kevin Buettner on Thu, 13 Jul 2023 14:50:47 -0700) Subject: Re: [PATCH v3 0/1] update MAINTAINERS file with git trailers References: <20230713105651.2281574-2-blarsen@redhat.com> <20230713145047.358e2c4a@f37-zws-nv> X-Spam-Status: No, score=1.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Level: * X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > Date: Thu, 13 Jul 2023 14:50:47 -0700 > From: 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 > > On Thu, 13 Jul 2023 12:56:51 +0200 > Bruno Larsen wrote: > > > Right now there is one big unanswered question: Should we have a > > specific tag to explicitly signal when a patch has been partially > > approved? Eli asked for it to avoid people mechanically reading tags > > from thinking that a patch has been fully approved when it was only > > partial. > > I don't think we need a tag for this. Since we review and/or approve > patches via email, I think some additional text stating which portions > were reviewed or approved is sufficient. > > Suppose I'm an area maintainer or a global maintainer who has confident > knowledge of a particular area. I might then do something like this: > > For the mn10300 architecture portions: > Approved-by: Kevin Buettner > > Only the Approved-by tag would be added to the git trailer, but it's > clear to anyone involved in the approval process that I haven't > approved the patch in its entirety, only certain parts. If I were to > review the rest of the patch, but not approve it, I see nothing wrong > with also saying: > > For everything else: > Reviewed-by: Kevin Buettner > > I also see nothing wrong with qualifying the 'Reviewed-by' or > 'Acked-by' tags. Yes, we might end up with a patchwork of reviews, > but we might also get more people involved with the review process, > which I think would be a good thing. > > If we really want to include the portions reviewed in the trailer, then > I suggest extending the format of the trailer, perhaps like this: > > Approved-by: Kevin Buettner (mn10300 only) The above will only work if everyone pays attention to those qualifications. Moreover, in Real Life, the response doesn't include just two such lines, it in many cases includes more text, and those qualifications can easily "drown" in that. Also, we used to have a way of saying "Approved, if those few nits are fixed", and the above either removes that possibility entirely, or will make it harder to determine whether and which parts were approved, and on what conditions. I still don't understand why we need the "partial-approved" facility that uses Approved-by. How is it different from Reviewed-by? the submitter still needs to figure out whether all the parts were okayed or not, so the only aspect this changes is making it more complicated for area maintainers to write these tags, because instead of just a single Reviewed-by they need to choose among two tags. So my vote is for reserving Approved-by only to the cases where the entire patch is approved. Alternatively, we could introduce an additional tag, like Partially-approved-by or something. I guess my point is that this should be simple and ideally include only fixed text, not some free-form text that could lead to misunderstandings and misinterpretations.