[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: r53484 - in trunk/packages/partman/partman-partitioning: debian free_space/new

On Tuesday 27 May 2008, Colin Watson wrote:
> > >  create_new_partition () {
> > > -	local num id size type fs path name mp mplist mpcurrent numparts
> > > device open_dialog NEW_PARTITION $type ext2 $freeid $place $size
> > > +	local num id size type fs path name mp mplist mpcurrent numparts
> > > device id=''
> > >  	read_line num id size type fs path name
> > >  	close_dialog
> > It looks wrong to me as now some vars are being defined as local after
> > they are first used.
> No, that isn't the case. ask_for_type sets the 'type' variable which is
> used at the top of create_new_partition. create_new_partition then
> reuses type for something entirely different (and localises it in the
> process).

Ah, so the vars in the 'device open_dialog' line are actually global vars 
set outside the function? That is very ugly coding for partman standards 
and should definitely be fixed.

> > Should the local line maybe be split into two lines instead?
> Probably wouldn't hurt to rename the type variable in
> create_new_partition, but there's nothing actually incorrect about my
> change as far as I can see.

It's not just 'type', it is 'size' as well.

My preference would be to remove type and size from the first 'local' 
declaration and have a second 'local' declaration after the open_dialog 
call with just those two vars. Probably add a comment as well.

Only problem with you change is that someone might be inclined to move the 
line back up...

Attachment: signature.asc
Description: This is a digitally signed message part.

Reply to: