[parted-devel] [PATCH 2/2] test for the bootcode-in-extended-partition fix

[Petr Uzel]

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/parted-devel@lists.alioth.debian.org/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: puzel at suse.cz
Lihovarská 1060/12                          http://www.suse.cz
190 00 Prague 9, CR                             
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20090826/d6dbc8de/attachment.pgp>