On Wednesday 28 May 2008, Philip Hands wrote: > > I've had a quick look, but after only a few items I have some doubts...
Let me explain why I had doubts. We are currently at the point where we should be stabilizing D-I for Lenny. We're supposed to be removing the last pesky and dumb code errors, not introducing new ones. The huge risk here is that some stupid little error is going to slip through in a little used codepath that does not get tested anymore before the release, but happens to break any install on type Y systems for arch X. Also, we have seen in the past that busybox can sometimes either not support some options or behave slightly differently, especially for more obscure (less used / advanced) options. So basically I agree with the concept, but have problems with the timing. However, that does not mean you cannot work on this. Especially as you use local a git-svn checkout, relatively small changes like this should keep quite well, even if not committed in the central repository. So let me suggest the following: - Commit your changes in git locally. - Use separate commits per patch or set of related patches (like the partman 'cut -c-N' ones). - Add meaningful commit messages, especially when a change is not trivial (as in the case of 'uniq -u'); don't bother with changelogs yet. - When you have a bunch of changes, sort them into separate branches for 'trivial, impossible that it breaks anything', 'probably OK, but not 100% sure', 'needs review/testing', 'changes or could change behavior'. The sorting can be done using 'git cherry-pick'. - The first category you could IMO just commit. We can review them based on the commit messages. - Anything other than the first category should IMO wait until after the Lenny release. We could possibly do the second with reviews, but I'd rather spend the energy on testing and functional changes we want in Lenny. - After Lenny it should be fairly trivial to rebase the changes against then current SVN. I'd expect very little fallout or conflicts (best avoid making loads of changes in a single larger block of code though). Accidental breakage is then a lesser concern. > That should be this instead: > wc -l < massbuild.versions > > "cat file | foo" can always be done more efficiently as "foo < file" > but I generally leave out the redirection if I can get away with it > (which of course in this case, I could not :-/ ) Agreed, though it always feels a bit like reading right to left or top posting to me. > well, we could go for the redirection approach instead, but I'd be very > surprised if you found any more such errors, as I'm pretty sure that the > other examples were well tested (except that I'll admit to having tested > them under busybox on a full Debian system, rather than in a d-i system > -- I'll test them all again under d-i proper if you think that there's a > danger that busybox varies in its behaviour (which I _seriously_ hope it > doesn't) See above. Please do check anything that is not already commonly used in D-I with busybox. I checked the 'cut -c-N' and 'uniq -u' and those seem OK. > > > - for dir in $( (cat /tmp/mount.pre /tmp/mount.pre; mountpoints ) | \ > > > - sort -r | uniq -c | grep "^[[:space:]]*1[[:space:]]" | \ > > > - sed "s/^[[:space:]]*[0-9][[:space:]]//"); do > > > + for dir in $(mountpoints | sort -r /tmp/mount.pre /tmp/mount.pre - > > > | uniq -u); do > > > > 1) I find the combination of using a pipe _and_ named files for sort > > unintuitive and less readable > > Yeah, I'll give you that -- I was wondering if that was such a good idea. > > > 2) Where did the grep and sed statements disappear to? > > they're a somewhat log-winded reimplementation of uniq -u OK. That could have been explained in a (git) commit message included in the patch. It was non-obvious to me (at least not within the time I want to spend on review). Having the comment would have avoided the question. > For improved readability, how about: > for dir in $( (cat /tmp/mount.pre /tmp/mount.pre; mountpoints) | \ > sort -r | uniq -u); do Yes, better. Maybe add a space before the last closing parenthesis too. Re some of the other changes in your original patch: - yaboot-installer: category 1 - floppy-retriever/debian/load-floppy.postinst In current script spaces get dropped by not quoting in the echo statements a bit lower down, so should be OK. Category 2. - localechooser - "redundant use of cat": no problem - 'wc -l': broken - partman: fine with me, needs good commit and changelog messages These statements should IMO already have had a comment. Can you add one? Maybe: "# Truncate label to N characters" If you need any help with git for this, feel free to ask. Cheers, FJP
signature.asc
Description: This is a digitally signed message part.
