Hi Kurt, I'm just reviewing this. Before I get started, could I ask you to add a few more details to your commit messages? Many things that look obvious to us now might not be obvious to future contributors, so I would prefer too many rather than to few comments.
What I have in mind: On Sun, Jul 3, 2011 at 10:31 PM, Kurt J. Bosch <[email protected]> wrote: > Kurt J. Bosch (13): > functions/rc.single: Clean up whitespace That's ok. > rc.sysinit: Fix stat_busy block indentation A sentence explaining the "policy" would be nice: "Align stat_{fail,done} with stat_busy". > functions: Get rid of superfluous braces in udevd_modprobe() Why are they superfluous? This has been pointed out on the list, and it is pretty basic, but I prefer if these kind of things are pointed out explicitly. > functions: Add missing quotes in mount_all() Why are they needed? It is nice to know the difference between "this is the right way to do it, even if it does not fix a bug" and a bug fix. > functions/rc.multi: Strip paths from binaries > rc.sysinit/functions: Refactor kill_everything/fsck_all/mount_all Those are good. > rc.sysinit: Declare $FORCEFSCK read-only to prevent custom hooks from > messing Fine I guess, but it would be better if you split the summary and justification in two lines: Declare $FORCEFSCK read-only This prevents <name of hooks> from overwriting the value. > functions/rc.sysinit: Refactor fsck-redirection to prevent breakage Good comments. Question: Is it necessary to declare the empty variables? > functions: Sanitize status() behaviour to make it more intuitive Good comments. I agree with returning the error code. Not sure about the redirection. If you have a usecase (not the one below), then maybe. > rc.sysinit: Simplify setting hostname by using status() even with > redirection now I don't think this works. Before this patch the status encapsulates the writing to /proc, after this patch it only encapsulates the echo command, so in case we cannot write to /proc the status message does not behave as intended. > functions: Deprecate in_array(), Add replacement is_in_array() I agree with changing to use ck_daemon, probably about the deprecation too, not sure about adding replacement function without a user though. > functions: Add stop_all_daemons() and related *_prestopdaemons hook > functions: Speed up reboot/shutdown by recognizing killall5 exit code > 2 Good comments, I'll have to look into the killall5 stuff before deciding though. Cheers, Tom
