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) > 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. And yes, Laszlo, you need some time off:-) -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
