On Tue, Jul 6, 2010 at 11:03 PM, Allan McRae <allan@archlinux.org> 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