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

Bug#564441: new version of the proposed patch

	Hello Frans,

Le Wed, 3 Feb 2010 22:54:01 +0100,
Frans Pop <elendil@planet.nl> a écrit :

> Hello Fred,
> It took a bit of time, but I think you'll see why.
> I do really like the new functionality. I think we'll need to discuss
> the details of the user interface a bit more, but the concept
> definitely works. My compliments for your work.
Thanks for the compliments, much appreciated :-)
> Attached a patch series, mostly on top of your last patch. It looks
> like a lot, but you'll see that I've not actually changed your
> functionality that much. A lot of the changes are cleanups and coding
> style improvements. Also, some of the changes are improvements of
> existing code/functionality and don't really change your code. I did
> fix a few issues; more below.
> …

I reviewed your patches, quite nothing to note, it's a nicer and
cleaner code (and I know that terse code doesn't mean maintainable
one ;-).
I followed your suggestion to use git (git-svn) to study it, and one
more time, I noted I have to learn more this great tool and all stuff
around it.

Here is my small remarks/questions :

  * nice trick with ${var:+} : devlist=${devlist:+$devlist, }$dev
  * what does mean ':sl3:' in debconf templates ? I didn't use the '_'
before 'description' because then doing it, then loading template inside
iso-scan.postinst (as a hack you shown me), I got empty strings.
Actually, I incorrectly put a ^ instead of _ just before building my

  * the 'printf' call simplifies the ISO description, well done.

  * ok for 'local' use (I didn't find one so assume it wasn't used
here). About variable names, I only guessed that uppercase names were
    for global variables, whereas lowercase were for local ones.

  * you're right for the bug. I wonder why we differentiate '.  and
first-level dirs ?

  * fixme about -e sufficient to detect broken symlinks : yes.
  * about using '-f' find flag to filter symlinks : I would not use
    it in first pass, to allow symlinks in the top dir pointing to a
sub-level one, but use it in second pass to avoid duplicates.

  * state 20 : why not choosing debconf level (between medium and high)
depending on number of ISO found, as in state 30 ?
  * state 30 : we ask user to choose which ISO to use even if there is
only one, isn't it ?

I thought also that the first argument for scan_device_for_isos
function should be a text arg like 'top'/'full' instead of 0/1,
we'd have better readability and don't loose speed.

One 'FIXme' you didn't add but spoke before was about ISO presentation
when selecting one : perhaps we could better present devices (with
disk name, partition labels if any) ?

  I'll try to look about other FIXme tags in the next days, and propose
patches if I can do nice code. I also have to test your version more
carefully in my Qemu setup, as first runs didn't run as expected, but
anyway, I'm using your version now.

    with regards,

Reply to: