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. -Dan
