On Sunday 23 December 2007, Stephen Gran wrote: > Please do _not_ apply this patch without review. This patch serves two > purposes: the first, and the one most useful to remember is that it > serves as a way to introduce me to the code base. The second purpose is > to reduce the size of the code base, and make things slightly more > unified and canonical. Thanks for the patch. Saving about 300 lines of code looks worth the effort. Have you followed the discussion on your original thread, especially the part about updating/adding dependencies on di-utils? Some comments on the patch. I've not done a full review, but I have taken a fairly close look :-) * I warned you about partman: it already has its own log() function and your patch conflicts with that (see partman-base/lib/base.sh). It's really best to initially leave all partman components out of this change. * Not sure if "output.sh" is a very logical name for this. Seems to general. * I'd define the functions in order of "severity": info before warning * The "die" function. That function seems somewhat out of place with the others and its name does not really indicate that it also displays an error dialog (maybe "exit_error" as it was called in some places would be better?). It is used in quite a few places, so factoring it out does seem logical. At least in one place (base-installer) we lose the error that was printed to syslog with the new function. I'm not really sure what to do here. * debian-installer-utils/in-target Not sure about the change there. Note the "--" in the command, which was added to avoid problems with some error strings. Does that still work? Should that maybe be included in the new log() function by default instead? * finish-install/finish-install.d/90console You mangled the indentation in the first hunk. * apt-setup That udeb is somewhat intended to also be used in installed systems (see the use of the $ROOT variable). Having its scripts depend on a d-i function library is therefore probably not a good idea. * os-prober Same, but that udeb already _has_ a regular package equivalent! * debian-installer-utils/chroot-setup.sh Note that this file itself is already sourced, so the chance is pretty high that we'll be sourcing the new file twice (at least from in-target). Not sure if that'd need fixing, but certainly worth a comment.
Attachment:
signature.asc
Description: This is a digitally signed message part.