Re: r53484 - in trunk/packages/partman/partman-partitioning: debian free_space/new
On Tue, May 27, 2008 at 01:42:23PM +0200, Frans Pop wrote:
> On Tuesday 27 May 2008, Colin Watson wrote:
> > 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.
It's ugly coding by d-i standards, but apparently not unusual in
partman. :-(
> > 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.
True.
> 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.
They aren't actually used as far as I can see, so I just replaced them
with x1, x2, etc.
> Only problem with you change is that someone might be inclined to move the
> line back up...
I added a comment to defend against that.
Thanks,
--
Colin Watson [cjwatson@debian.org]
Reply to: