From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7F7D320957AFB for ; Wed, 2 May 2018 06:40:09 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5573C4270965; Wed, 2 May 2018 13:40:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-197.rdu2.redhat.com [10.10.120.197]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8BF2063529; Wed, 2 May 2018 13:40:07 +0000 (UTC) To: Evan Lloyd , "ruiyu.ni@intel.com" Cc: "edk2-devel (edk2-devel@lists.01.org)" References: <69dccdf5-9397-3bdc-0b89-66361a685719@redhat.com> From: Laszlo Ersek Message-ID: <948ba2ee-1f88-a346-14c8-815fb9b835c8@redhat.com> Date: Wed, 2 May 2018 15:40:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 02 May 2018 13:40:08 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 02 May 2018 13:40:08 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: Incorrect Author on patch X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 13:40:10 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/02/18 14:46, Evan Lloyd wrote: > One obvious way of precluding this sort of problem would be to move to > using the pull request mechanism on GitHub, rather than requiring > maintainers to play with upstreaming local patches. In my opinion: - *git* pull requests: yes, - *github* pull requests: please no. We discussed this a few years (?) ago, and I believe the outcome was that pulling remote branches was fine, as long as: (a) the location to pull from was noted in the cover letter of a patch series, or in a separate pull request emailed to the list, and (b) the maintainer pulling the remote branch verified that the patches reviewed on the list were formatted from the commits on the branch being pulled. The remote branch being pulled may well reside on github, but the review must occur on the list (not on the GitHub web UI), and the actual fetch & merge actions must be carried out by a maintainer locally in their work environment, from where they are supposed to push the new commits to the central repo. In other words, we must not start relying on GitHub tools for either patch reviews or for generating merge commits (or any other kinds of commits). > I can well understand why it would be useful to use Gerrit as a means > of reviewing a patch - actually a brilliant idea, (actually, *not* a brilliant idea, but that's just my opinion :) ) > but it does introduce problems. If, post review, the merge were to be > from a pull request then there would be no risk of "meta-data > corruption". Agreed 100% about "no risk of meta-data corruption". However there is a new risk, namely that of getting unreviewed code into the tree. See requirement (b). And I can imagine that such verification is more trouble for some maintainers than simply applying the patches from the list. Basically, the way I would do it, is: - identify the branch fork-off point from the pull request, - create a local branch at that point, - apply the patches from the mailing list to the local branch, - fetch the remote branch, - format the patches from the local branch, - format the patches from the remote branch, - compare the two sets of locally-formatted patches, - eyeball the results. Local reformatting of the patches applied from the mailing list is necessary because formatting options (such as context size, file order, diff algorithm, subject prefix) affect the comparison. And, simply git-diffing the commits on the parallel branches isn't sufficient, because it doesn't compare authorship or even commit messages. In other projects, pulling from someone basically means letting that someone commit to the project unsupervised. So it's a position of trust. Hence signed pull requests. And those people that submit the pull requests, they certainly need to apply the "leaf" patches from mailing lists, from other contributors, after review. All in all, I don't think there's a way around applying patches intact from the mailing list. Whatever workflow we choose, that action will be part of that workflow. Thanks Laszlo