From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 67DD11F5A1 for ; Tue, 12 Nov 2019 20:40:13 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:cc:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=qjlnI13jyQP1Gopv DOouEYSANhoUwXBZFee5B7d6sXVvnaKGvnuh7m/lJw/zmvpk2Rdrqv2Wi8GCvbLw 9qnzHDfpWDUkmBLCVQEe0E57Is//CssZN8i9cWER98VV1ezzRhSQE3sH+7TmTmP5 cWX3CJuAYN77FiknmUB6DlViqts= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:cc:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=L5km8RssxZV+a7RQ3aQ8KC /PpzI=; b=ZjtJxm50INYldj8BQ4OG/j5X+4cl695Ddw7BjieTnpJS21AVZpk4a8 GWlXufZOWoPZ9dgN2sQJJ0iV4/AotFlgzMcdOa6BasSvT374lpXnZIswENzLKxvp BrVBbA02xHh0aacoLD12p8iG585k8XepUwhuXeSB8NlTmGcUQOC6k= Received: (qmail 25149 invoked by alias); 12 Nov 2019 20:40:11 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 25141 invoked by uid 89); 12 Nov 2019 20:40:10 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: us-smtp-1.mimecast.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1573591207; 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=6FMTT9urUh7kh20te3sCfPzN+0Sv+eD6/uqBEwFzhwQ=; b=au6HtEPgQpo4uIn9xQZ+Sp3znOumf3qEyNNomyisWKHJL2dorW0As7gsIRbcI7jjMV50ES 3Pb0km0M/rK4r1QeD/kgjy5wM/pKB+Cxp0k79wcZrG5nyg6qp1JySZ12nMvZdiOGiWv7mJ eYXJjJYeVNyb47owkEAYwQVY4PRXKQ8= Subject: Re: How to keep Reviewed-by lines in git commits with gerrit. To: Florian Weimer Cc: libc-alpha References: <7b4aa5d2-14e7-c0aa-a258-bbd60455fae5@redhat.com> <87lfskgbgw.fsf@mid.deneb.enyo.de> <593d3aac-c38e-4d01-2652-41ec5442d062@redhat.com> <87h838ga00.fsf@mid.deneb.enyo.de> From: Carlos O'Donell Message-ID: <725f62b0-b6bc-bbbc-0b8f-f22e29eaeb3a@redhat.com> Date: Tue, 12 Nov 2019 15:40:03 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <87h838ga00.fsf@mid.deneb.enyo.de> X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 11/12/19 2:39 PM, Florian Weimer wrote: > * Carlos O'Donell: >=20 >>> We should not edit the commit message manually after it has been >>> reviewed. >> >> I agree. Should we setup gerrit to push to glibc git? >=20 > I think it's to early for that. I'm not sure that Gerrit is the right > tool for us. Too many of us have yet to use it as patch submitters > and reviewers. Is it too early though? Enabling gerrit to push would not stop others from pushing to master. It would just enable the tool to push to glibc git itself after reaching a +2 review rating. And we could limit who can give the +2 reviews by setting up groups accordingly. I agree that more people should try it out as patch submitters and reviewers. I agree it's too early to tell if this is the tool we want to add to our review toolbox. > I also think that improving the email interface is a dead end and not > worth the effort. Unless we are willing to abandon email-based patch > review, we should not switch to Gerrit (or any other web-based tool). I agree that making gerrit suit *all* of our email needs is a dead end. I do not think we will ever abandon email-based patch review. I do not think that having email-based patch review blocks our use of another tool for patch review. Yes, it creates 2 tools, email and another tool, as valid ways to submit patches, but I don't see that as a problem. I think we should also be looking at Patchwork2. > Gerrit doesn't have to be in push mode in order to edited commit > messages. You can press the DOWNLOAD button, copy the cherry-pick > command line into a shell, and use that to push a commit to master. > I've been doing that a couple of times and it works well. Yes, but in push mode it can automatically add Reviewed-by: lines if the reviewers sign off with +1 or +2. >>> Either it is automatically added by the review tool, or we need to >>> gather patch review statistics from the review tool. >> >> I would prefer it to be automatically added by the review tool, as >> gerrit is designed to do. >=20 > Is it? If we need too many customizations, we'll be stuck with our > own special pet, which I don't want. It is. I agree about not wanting to own our own special version of gerrit. It is specifically designed to be able to do this. https://twitter.com/CarlosODonell/status/1192563350191902723 I see Jonathan mentioned this already. >>> The latter has the advantage that post-commit review also counts, >>> which is not possible if we only consider Reviewed-By: lines in >>> commits. >> >> Post-commit review is indeed lost in a system that uses commit >> messages to track review, but I'd argue that this is a such a small >> minority of the work that it's not worth accurately modeling in >> the framework. >=20 > I'm not sure about that. In many cases, I've missed Reviewed-By: > lines due to lack of tool support. But there have been a few where I > simply had not received the review email message when producing the > actual commit. It's hard to objectively quantify because the data is hard to get. The only thing we can do is use a system that avoids it by adding the reviewed-by lines automatically, like gerrit can do? Staying on email-only patch reviews we will simply continue to have the same issues. >> The reason I'm strongly in favor of Reviwed-by: in commit messages >> is also that the review data goes with the commit, and clones of the >> repo for analysis by any other 3rd party with normal git tooling. >=20 > I would argue this is a fringe use case. How is this relevant to the > glibc project, anyway? It's not that we're going to promote > contributors due to new roles based on the number of reviews they > conducted. I'm not quite sure what you argue is a fringe use case? The point I made about 3rd parties? In general I see 3rd parties as my manager doing a review of what work I'm doing upstream. People not involved in the project directly. Why I want "Reviewed-by:" : (a) Preserving the review attributions. This history is something we have worked hard to preserve either through ChangeLogs, CVS, or git. Having it in a distinct place like gerrit's metadata may cause it to be lost (which is why I want records on the mailing lists of the the reviews). The only real things which stand the test of time so far are the mailing list archives, and the commit messages (and not even that sometimes). I might argue the reviewed-by should also go into the ChangeLog messages we generate (maybe it will). How is this relevant to glibc? To the project itself? (b) The relevance of recording reviewers. Firstly, for me, it allows me to show my employer that I'm doing something upstream. Likewise for other employees doing reviews. These are objective ways to measure your reviews are helping the project. It is a *measurable* thing, that can be measured independently by a manager in a very quick way. Secondly, it is an objective metric to look at project health and to see how many reviewers there are working in the project (without needing to parse emails manually). I look at Reviewed-by at every release to see who is reviewing and if I've been increasing my own numbers of reviews to help patch backlogs. (c) The relevance of a Reviewed-by: line. I'm not particularly tied to using Reviewed-by: lines in commit messages, but I'd like some way to get the data and have it go with the commit so we don't loose the data (see (a)). (d) Taking action on what we see. I was initially worried that we had few reviewers, but this year alone we have 25 unique recorded reviewers. That's much better than I expected. Next I want to see about encouraging those that reviewed when their reviews were fewer than 1 a month, and I thought it might be a good idea. Perahaps these reviewers might be interested in mentoring. Likewise contributors wit= h zero recorded reviews might be good candidates for mentoring also. --=20 Cheers, Carlos.