First of all: Sorry that I haven't finished the review, I have been busy with work and the heat kept me from thinking clearly. Am 07.07.2010 06:25, schrieb Dan McGee: > On Tue, Jul 6, 2010 at 11:03 PM, Allan McRae <allan@xxxxxxxxxxxxx> wrote: >> Here is a quick review on all these patches. I recommend that the lvm and >> crypttab changes get a decent amount of testing before these go live as they >> are the biggest changes being done. >> >> Why has this been removed: >> -if [ -x /etc/rc.local.shutdown ]; then >> - /etc/rc.local.shutdown >> -fi >> Ah... it has been moved to another place in another commit. Please >> document these sorts of changes in your commit message and preferably do the >> entire move in one commit. > > If there was one thing I wasn't so fond of in these patches, it seemed > like there were too many. The beauty of git is the ability to go back > and squash and split patches in a way that makes a lot more sense to > others- it might not have been the way or order you did things in, but > you should try as hard as possible to make a commit the largest > logical unit that makes sense, but still small enough to grasp fully. > > If there are ever closely-related changes strewn across multiple > patches in a patch set, you should probably think about merging those > commits. I agree with Dan here. For example, all the commits that are merely "replace [ with [[" and no functional changes should be one commit only. I will release new initscripts and mkinitcpio now with a fix that needs to go to core before we do such a major change in initscripts.
Attachment:
signature.asc
Description: OpenPGP digital signature
