Re: [edk2] Incorrect Author on patch
On 5/2/2018 7:40 AM, Laszlo Ersek wrote: On 05/02/18 14:46, Evan Lloyd wrote: 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 :) ) *Please* not Gerrit. If we're going to use a code review system, please let us choose Phabricator. It has a much nicer user interface. You can see an example of the review requests it creates at https://reviews.freebsd.org/differential/ . -- Rebecca ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Incorrect Author on patch
Evan:
I agree this is a mistake in edk2 project. We mix the operation in edk2
project and our internal project. We will double check the patch and avoid such
case happen again.
Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Evan
> Lloyd
> Sent: Wednesday, May 2, 2018 8:46 PM
> To: Laszlo Ersek <ler...@redhat.com>; Ni, Ruiyu <ruiyu...@intel.com>
> Cc: edk2-devel (edk2-devel@lists.01.org) <edk2-devel@lists.01.org>
> Subject: Re: [edk2] Incorrect Author on patch
>
> 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. I can well
> understand why it would be useful to use Gerrit as a means of
> reviewing a patch - actually a brilliant idea, 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".
>
> Regards
> Evan
>
> > -Original Message-
> > From: Laszlo Ersek <ler...@redhat.com>
> > Sent: 25 April 2018 22:15
> > To: Evan Lloyd <evan.ll...@arm.com>; ruiyu...@intel.com
> > Cc: edk2-devel (edk2-devel@lists.01.org) <edk2-devel@lists.01.org>
> > Subject: Re: [edk2] Incorrect Author on patch
> >
> > On 04/25/18 17:02, Evan Lloyd wrote:
> > > Hi Ruiyu.
> > > When we look at the edk2 git log, or GitHub
> > >
> > https://github.com/tianocore/edk2/commit/ee4dc24f57c32a445e7c747396c
> > 9b
> > > fbd8b221568 we see that Sami's AcpiView patch shows you as the Author.
> > > I'm not sure what might have caused that (and it is obviously
> > > accidental), but
> > it is a little unfortunate in that the commit doesn't show up on Sami's
> > GitHub
> > stats.
> > > Fortunately, this is not currently significant for our organisation,
> > > although I'm
> > sure Sami would prefer to have the credit.
> > > This is a trivial matter, but you may wish to understand what caused it
> > > so that
> > you don't accidentally upset someone for whom the stats are significant.
> >
> > Right, this occurred at least one other time as well: see commit
> > 8b5c80e0296c ("MdeModulePkg/UefiBootManagerLib: fix
> > AddLoadOptionVariable docs/prototype", 2018-04-23). Ray pushed the patch
> > (so the Committer field is certainly right), but Ray's name+email replaced
> > mine
> > in the Author field. I had noticed that earlier, but now I'm seeing it as a
> > pattern.
> >
> > I believe this is a tooling issue. I notice the following bit on the commit
> > message:
> >
> > Change-Id: I8a609d6502b6f8929b2f568acaa147065003b6f4
> >
> > I certainly didn't post the patch with that, and I doubt Ray added it
> > manually.
> > So, whatever tool Ray used to handle the patch lost the authorship
> > information.
> >
> > And, I see the exact same kind of tag, namely
> >
> > Change-Id: Ifa23dc80ab8ab042c56e88424847e796a8122a7c
> >
> > on commit ee4dc24f57c3 ("ShellPkg: Add acpiview tool to dump ACPI tables",
> > 2018-04-23), which you mention.
> >
> > ... In fact, my patch happens to be the direct ancestor of Sami's, in the
> > git
> > history, and their commit times are just ~3 minutes apart. I'm quite
> > certain Ray
> > has started using a new tool.
> >
> > For example, commit bc2288f59ba2 ("UefiCpuPkg/MpInitLib: put
> > mReservedApLoopFunc in executable memory", 2018-03-08) was also
> > committed by Ray, *but* Jian's authorship was preserved fine. (No "Change-
> > Id" either.)
> >
> > ... Oh... it looks like those Change-Id's were added by Gerrit:
> >
> > https://git.eclipse.org/r/Documentation/user-changeid.html
> >
> > And then, please look at this:
> >
> > https://gerrit-review.googlesource.com/Documentation/error-invalid-
> > author.html
> >
> > For every pushed commit Gerrit verifies that the e-mail address of
> > the author matches one of the registered e-mail addresses of the
> > pushing user. If this is not the case pushing the commit fails with
> > the error message "invalid author". This policy can be bypassed by
> > having the access right 'Forge Author'.
> >
> > Uh, what?... Gerrit says "invalid author" if Ray pushes a patch that wasn't
> > authored by himself? And the option to override that is called "forge"
> > author?
&
Re: [edk2] Incorrect Author on patch
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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Incorrect Author on patch
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. I can well understand why it would be useful
to use Gerrit as a means of reviewing a patch - actually a brilliant idea, 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".
Regards
Evan
> -Original Message-
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: 25 April 2018 22:15
> To: Evan Lloyd <evan.ll...@arm.com>; ruiyu...@intel.com
> Cc: edk2-devel (edk2-devel@lists.01.org) <edk2-devel@lists.01.org>
> Subject: Re: [edk2] Incorrect Author on patch
>
> On 04/25/18 17:02, Evan Lloyd wrote:
> > Hi Ruiyu.
> > When we look at the edk2 git log, or GitHub
> >
> https://github.com/tianocore/edk2/commit/ee4dc24f57c32a445e7c747396c
> 9b
> > fbd8b221568 we see that Sami's AcpiView patch shows you as the Author.
> > I'm not sure what might have caused that (and it is obviously accidental),
> > but
> it is a little unfortunate in that the commit doesn't show up on Sami's GitHub
> stats.
> > Fortunately, this is not currently significant for our organisation,
> > although I'm
> sure Sami would prefer to have the credit.
> > This is a trivial matter, but you may wish to understand what caused it so
> > that
> you don't accidentally upset someone for whom the stats are significant.
>
> Right, this occurred at least one other time as well: see commit
> 8b5c80e0296c ("MdeModulePkg/UefiBootManagerLib: fix
> AddLoadOptionVariable docs/prototype", 2018-04-23). Ray pushed the patch
> (so the Committer field is certainly right), but Ray's name+email replaced
> mine
> in the Author field. I had noticed that earlier, but now I'm seeing it as a
> pattern.
>
> I believe this is a tooling issue. I notice the following bit on the commit
> message:
>
> Change-Id: I8a609d6502b6f8929b2f568acaa147065003b6f4
>
> I certainly didn't post the patch with that, and I doubt Ray added it
> manually.
> So, whatever tool Ray used to handle the patch lost the authorship
> information.
>
> And, I see the exact same kind of tag, namely
>
> Change-Id: Ifa23dc80ab8ab042c56e88424847e796a8122a7c
>
> on commit ee4dc24f57c3 ("ShellPkg: Add acpiview tool to dump ACPI tables",
> 2018-04-23), which you mention.
>
> ... In fact, my patch happens to be the direct ancestor of Sami's, in the git
> history, and their commit times are just ~3 minutes apart. I'm quite certain
> Ray
> has started using a new tool.
>
> For example, commit bc2288f59ba2 ("UefiCpuPkg/MpInitLib: put
> mReservedApLoopFunc in executable memory", 2018-03-08) was also
> committed by Ray, *but* Jian's authorship was preserved fine. (No "Change-
> Id" either.)
>
> ... Oh... it looks like those Change-Id's were added by Gerrit:
>
> https://git.eclipse.org/r/Documentation/user-changeid.html
>
> And then, please look at this:
>
> https://gerrit-review.googlesource.com/Documentation/error-invalid-
> author.html
>
> For every pushed commit Gerrit verifies that the e-mail address of
> the author matches one of the registered e-mail addresses of the
> pushing user. If this is not the case pushing the commit fails with
> the error message "invalid author". This policy can be bypassed by
> having the access right 'Forge Author'.
>
> Uh, what?... Gerrit says "invalid author" if Ray pushes a patch that wasn't
> authored by himself? And the option to override that is called "forge" author?
> O_o
>
> Anyway, the page continues,
>
> If pushing to Gerrit fails with the error message "invalid author"
> and somebody else is author of the commit for which the push fails,
> then you have no permissions to forge the author identity. In this
> case you may contact the project owner to request the access right
> '+1 Forge Author Identity' in the 'Forge Identity' category or ask
> the maintainer to commit this change on the author’s behalf.
>
> Ray, if you absolutely must use Gerrit, please make sure that you have the '+1
> Forge Author Identity' access right. IMO, we maintainers are responsible for
> preserving git metadata the best we can.
>
> (For example, if a patch is applied from the mailing list with "git am", it
> preserves the authorship information -- the documentation says, 'The commit
> author name is taken from the "From: " line of the message, and commit
> author date is taken from the "Date: " line of t
Re: [edk2] Incorrect Author on patch
On 04/25/18 17:02, Evan Lloyd wrote:
> Hi Ruiyu.
> When we look at the edk2 git log, or GitHub
> https://github.com/tianocore/edk2/commit/ee4dc24f57c32a445e7c747396c9bfbd8b221568
> we see that Sami's AcpiView patch shows you as the Author.
> I'm not sure what might have caused that (and it is obviously accidental),
> but it is a little unfortunate in that the commit doesn't show up on Sami's
> GitHub stats.
> Fortunately, this is not currently significant for our organisation, although
> I'm sure Sami would prefer to have the credit.
> This is a trivial matter, but you may wish to understand what caused it so
> that you don't accidentally upset someone for whom the stats are significant.
Right, this occurred at least one other time as well: see commit
8b5c80e0296c ("MdeModulePkg/UefiBootManagerLib: fix
AddLoadOptionVariable docs/prototype", 2018-04-23). Ray pushed the patch
(so the Committer field is certainly right), but Ray's name+email
replaced mine in the Author field. I had noticed that earlier, but now
I'm seeing it as a pattern.
I believe this is a tooling issue. I notice the following bit on the
commit message:
Change-Id: I8a609d6502b6f8929b2f568acaa147065003b6f4
I certainly didn't post the patch with that, and I doubt Ray added it
manually. So, whatever tool Ray used to handle the patch lost the
authorship information.
And, I see the exact same kind of tag, namely
Change-Id: Ifa23dc80ab8ab042c56e88424847e796a8122a7c
on commit ee4dc24f57c3 ("ShellPkg: Add acpiview tool to dump ACPI
tables", 2018-04-23), which you mention.
... In fact, my patch happens to be the direct ancestor of Sami's, in
the git history, and their commit times are just ~3 minutes apart. I'm
quite certain Ray has started using a new tool.
For example, commit bc2288f59ba2 ("UefiCpuPkg/MpInitLib: put
mReservedApLoopFunc in executable memory", 2018-03-08) was also
committed by Ray, *but* Jian's authorship was preserved fine. (No
"Change-Id" either.)
... Oh... it looks like those Change-Id's were added by Gerrit:
https://git.eclipse.org/r/Documentation/user-changeid.html
And then, please look at this:
https://gerrit-review.googlesource.com/Documentation/error-invalid-author.html
For every pushed commit Gerrit verifies that the e-mail address of
the author matches one of the registered e-mail addresses of the
pushing user. If this is not the case pushing the commit fails with
the error message "invalid author". This policy can be bypassed by
having the access right 'Forge Author'.
Uh, what?... Gerrit says "invalid author" if Ray pushes a patch that
wasn't authored by himself? And the option to override that is called
"forge" author? O_o
Anyway, the page continues,
If pushing to Gerrit fails with the error message "invalid author"
and somebody else is author of the commit for which the push fails,
then you have no permissions to forge the author identity. In this
case you may contact the project owner to request the access right
'+1 Forge Author Identity' in the 'Forge Identity' category or ask
the maintainer to commit this change on the author’s behalf.
Ray, if you absolutely must use Gerrit, please make sure that you have
the '+1 Forge Author Identity' access right. IMO, we maintainers are
responsible for preserving git metadata the best we can.
(For example, if a patch is applied from the mailing list with "git am",
it preserves the authorship information -- the documentation says, 'The
commit author name is taken from the "From: " line of the message, and
commit author date is taken from the "Date: " line of the message.')
Thank you!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] Incorrect Author on patch
Hi Ruiyu. When we look at the edk2 git log, or GitHub https://github.com/tianocore/edk2/commit/ee4dc24f57c32a445e7c747396c9bfbd8b221568 we see that Sami's AcpiView patch shows you as the Author. I'm not sure what might have caused that (and it is obviously accidental), but it is a little unfortunate in that the commit doesn't show up on Sami's GitHub stats. Fortunately, this is not currently significant for our organisation, although I'm sure Sami would prefer to have the credit. This is a trivial matter, but you may wish to understand what caused it so that you don't accidentally upset someone for whom the stats are significant. Regards, Evan Evan Lloyd | Technical Lead for Windows on Arm | Arm . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . m. +44 (0)7825256132 110 Fulbourn Road, Cambridge, CB1 9NJ Arm.com IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
