Sounds like a good plan. I am fine to hold CR0.WP to next week. Let's find root cause of failure.
I will repost them. Please give me some time to work out that. Again, thanks to educate me on the "bisectable way" -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Friday, November 27, 2015 8:07 PM To: Yao, Jiewen Cc: Ard Biesheuvel; edk2-devel-01; Leif Lindholm (Linaro address) Subject: Re: [edk2] please DO NOT commit unreviewed patches to subversion! On 11/27/15 12:31, Yao, Jiewen wrote: > Hi Laszlo and Ard > First of all, I apologize the confusing brought. > You are right. I am still using SVN to commit patch, until it is > finally moved to GIT. :-( The patch is reviewed and I did adopt the > suggestion from reviewer, before I finally commit it, after 24hours. > I am sorry that I did not aware the difference of commit between SVN and GIT. > > Laszlo > I am fine to revert them and commit in the way reviewed. I appreciate your > help! Thank you very much. I have now reverted the two patches in question, in SVN. I would like to request the following: (1) Please repost the final, updated, and reviewed version of your patch series Add 2 APIs in SmmCpuFeaturesLib to the edk2-devel mailing list. Please include Mike's Reviewed-by tags in the commit messages, and also please place the REPOST word in the brackets: [PATCH REPOST ...] I will then apply and commit these patches from the list, individually, to SVN. My latest (v5) OVMF SMM series has been adapted to the two new SmmCpuFeaturesLib API's, and these patches cause no regression, so I would actually like to commit them (individually) as soon as possible. (2) Please also repost your other patch series, in its final, updated, reviewed form: Always set WP in CR0. Please also include any Reviewed-by tags that you have received for it, and also place the REPOST word in the brackets. *Unlike* the other series, I would not like to (re)apply these patches immediately. First I would like to work with you and Paolo on tracking down the issue that they cause with KVM. I believe this series should be committed to SVN sometime next week, after my OVMF SMM work (v5) goes in. Thank you! Laszlo > > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, November 27, 2015 6:33 PM > To: Ard Biesheuvel; Yao, Jiewen > Cc: edk2-devel-01; Leif Lindholm (Linaro address) > Subject: Re: [edk2] please DO NOT commit unreviewed patches to subversion! > > On 11/27/15 10:07, Ard Biesheuvel wrote: >> On 26 November 2015 at 23:23, Laszlo Ersek <ler...@redhat.com> wrote: >>> I notice that recently there have been patches committed to the >>> subversion repository that had *never* been posted to the list, in >>> the ultimately committed form. Examples: >>> >>> (1) Patches on the list: >>> >>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/4770 >>> >>> [edk2] [patch 1/3] UefiCpuPkg/PiSmmCpu: Add 2 APIs in SmmCpuFeaturesLib. >>> [edk2] [patch 2/3] UefiCpuPkg/PiSmmCpu: Add NULL func for 2 new APIs in >>> SmmCpuFeaturesLib. >>> [edk2] [patch 3/3] UefiCpuPkg/PiSmmCpu: Update function call for 2 new >>> APIs in SmmCpuFeatureLib. >>> >>> Committed patch (one patch!): >>> >>> http://sourceforge.net/p/edk2/code/18958 >>> Add 2 APIs in SmmCpuFeaturesLib. >>> >>> The series that had been posted to the list, and reviewed there, in >>> a nice and structured way, got squashed into a single patch, and >>> commited that way. >>> >>> (2) Patches on the list: >>> >>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/4834 >>> >>> [edk2] [patch 1/2] UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page >>> table by default. >>> [edk2] [patch 2/2] UefiCpuPkg/PiSmmCpu: Always set WP in CR0. >>> >>> Committed patch (one patch!): >>> >>> http://sourceforge.net/p/edk2/code/18960 >>> Always set WP in CR0. >>> >>> Exactly same treatment. >>> >> >> I would argue that these patches need to be reverted, and committed >> in their final reviewed form. That way, we preserve the bisectability >> of the tree (since what was added as a single patch is removed as a >> single patch as well) > > In fact, because one of the patches above causes a regression for my OVMF SMM > work -- and because for tracking it down with good granularity we would > actually depend on the bisection --, I would propose to revert the above > (squashed) patches now, then wait until my series gets committed (this should > happen until mid next week). Then look at the individual patches again. > > I obviously offer my help in the bug hunt (which could finger a bug in QEMU > or KVM too), and I also offer to commit the final patches in their broken-out > forms, with "git svn dcommit", from the mailing list. > > Jiewen, under these circumstances, do you agree that the above two SVN > commits should be reverted? > > If so, I could do that as well; your call. > >>> This practice abuses the trust of the community and makes patch >>> review *absolutely pointless*. Patches must be committed to SVN >>> *exactly* the way they were posted and reviewed, no ifs and buts, >>> *unless* a reviewer gives explicit license to fix up trivial stuff right >>> before committing. >>> >>> *Maybe* if a trivial conflict has to be fixed via a rebase -- >>> because some light changes have crept in, between receiving reviews >>> and committing -- than can be done, but even in that case, >>> publicizing that fact on the list is the minimally expected courtesy. >>> >>> Squashing patches for committing is untolerable. It defeats the >>> original structuring, prevents bisection and bug hunt, and prevents >>> retroactive review and analysis. >>> >>> If I post a 30-50 part series, should I then commit it all in a >>> single >>> 3000-5000 SLOC bomb? That practice is *exactly* what we've been >>> trying to extinguish!!! >>> >> >> I could not agree more. The importance of being able to bisect is >> paramount, especially since not everyone takes the trouble to test >> their changes on platforms or toolchains other than the ones they use >> themselves, often resulting in breakage that needs to be tracked down >> by someone else. So *please*, post *and* merge your patches in a >> reviewable *and* bisectable way. > > I agree. > > I did realize however that maybe this was again a tooling issue. > Consider: the developers now mostly work with git (awesome! \o/ ), post their > patches that way, and so on. However, how do they commit from git to SVN? I > assume their git clones come from the mirror on github, *not* git-svn. So > they cannot use "git-svn dcommit" directly. > > The solution here (until we move to git completely) is, for each individual > developer, to have two git clones on the local system: one of the SVN repo, > made with git-svn, and another of the git mirror on github, simply made with > git. > > Then, for committing to SVN, patches have to be locally formatted out of the > "real" git clone (for posting anyway), applied to the git-svn repo, and then > committed from there with "git svn dcommit". > > (I use a somewhat reverse process: I work in my git-svn clone > primarily, and only use the github clone for pushing my development > branches, for review & testing purposes.) > > I guess this is not very intuitive, which is why (I assume!) Jiewen must have > grabbed a cumulative diff across the individual patches in git, and then > committed that (with a manually rewritten commit message) to SVN, using the > "svn" utility. > > That's not good. The idea with "git svn" is not just fetching SVN commits > into a local git clone; the reverse direction is possible and recommended as > well. > > Jiewen, if this is problematic to integrate into your workflow at the > moment, I would gladly help committing your patches to SVN the way > they are reviewed. (I would like to test your SMM patches with OVMF > going forward anyway -- so I'll have to apply them locally no matter > what.) > >> And yes, Laszlo, you need some time off:-) > > Yeah. Sorry. :( > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
