On Wed, Aug 26, 2009 at 11:00:38AM +0200, Jim Meyering wrote: > >> > * tests/t2300-dos-label-extended-bootcode.sh: New file. > >> > * tests/Makefile.am (TESTS): Add t2300-dos-label-extended-bootcode.sh. > >> Hi Petr,
Hi Jim, listmates, > >> > >> This looks good. > >> A few suggestions below. > > > > Hmm, actually in the first version of the patch there was this variable, but > > I was convinced not to use it: > > http://www.mail-archive.com/[email protected]/msg02450.html > > Don't succumb to requests to unfactor your code ;-) > However, his comment about keeping line length < 80 > and judicious variable names is a good one. > If long variable names push too many line lengths beyond 80 (each of which > requires a backslash and continued line, thus *decreasing* readability), > then consider a shorter variable name, but with a comment describing it. > It's always a trade-off. Sure, I can only hope that the quality of my patches will increase over time :) > > At least in this case, consistently using a variable for "440" > might have avoided the typo of "400". Agreed. BTW, in the new patches, I'll increase the value from 440 to 446 to make it consistent with t0202-gpt-pmbr.sh test. > >> > >> Why do you use /dev/urandom? > >> That might not exist. How about just using e.g., if=Makefile ? > > > > if=Makefile does not work, but if=../Makefile works. BTW the same issue is > > in t0202-gpt-pmbr.sh > > Good point. > It's easy to forget that each test is run in a subdirectory. > Considering that, it would be simpler (and slightly more maintainable, > in case the framework ever changes) to use a file that you know exists > in the current directory. So how about this: > > 'printf %0400d 0 > in && > dd if=in of=$dev bs=1c seek=16384 count=400 \ Yes, this is better. I've used the trick in new version - also for t0202-gpt-pmbr.sh. Patches will follow soon. As always, thanks for the comments. -- Best regards / s pozdravem Petr Uzel, openSUSE Community Multiplier Team ----------------------------------------------------------------- SUSE LINUX, s.r.o. e-mail: [email protected] Lihovarská 1060/12 http://www.suse.cz 190 00 Prague 9, CR
pgpoN3uckbWKU.pgp
Description: PGP signature
_______________________________________________ parted-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/parted-devel
