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! -----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
