On 11/27/15 13:12, Yao, Jiewen wrote: > Sounds like a good plan. > > I am fine to hold CR0.WP to next week. Let's find root cause of failure.
Right. > I will repost them. Please give me some time to work out that. Absolutely -- the local time is 13:17 for me, so you have plenty of time, as far as I'm concerned. If I get the "Add 2 APIs in SmmCpuFeaturesLib" series until midnight, that's okay. So please take your time and post them carefully. If you think it would also be useful to retest the patches, I would certainly understand and/or encourage that. Don't rush it. > Again, thanks to educate me on the "bisectable way" Yeah, it's not alway intuitive, especially that we have both SVN and git. Thank you for your quick reaction and understanding! :) Laszlo > > > -----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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
