Petr Uzel wrote: > On Fri, Aug 21, 2009 at 05:15:34PM +0200, Jim Meyering wrote: >> Petr Uzel wrote: >> > * tests/t2300-dos-label-extended-bootcode.sh: New file. >> > * tests/Makefile.am (TESTS): Add t2300-dos-label-extended-bootcode.sh. >> >> Hi Petr, >> >> This looks good. >> A few suggestions below. > > Hi Jim, > Thanks for the comments. > >> > +require_512_byte_sector_size_ >> > + >> > +# Note: the bootcode size is 440B >> >> Rather than that comment, please just define a well-named variable, >> >> bootcode_size=440 >> >> And then use it in place of the two hard-coded constants 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. At least in this case, consistently using a variable for "440" might have avoided the typo of "400". >> > + >> > +test_expect_success \ >> > + 'Install fake bootcode' \ >> > + 'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=400 \ >> >> 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 \ >> Shouldn't the above count=400 be count=440 (or now, count=$bootcode_size)? > > Yep, looks like a typo. > > I'll post a patch to address these issues. Thanks! _______________________________________________ parted-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/parted-devel
