On Sun, Jul 11, 2010 at 4:45 AM, Thomas Bächler <thomas@xxxxxxxxxxxxx> wrote: > 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. Going back through and rewriting the patch series to do that will be jsut about as much effort as creating the original one was. If I had know that was what your preference was, I would have written it that way to begin with. Since the end result is going to be the same once the patch series is applied, though, I don't really see the point of making modifications beyond bugfixes. > 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. > >
