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

Bug#447611: update-initramfs triggerisation



thanks for your quick reply.

On Tue, 23 Oct 2007, Ian Jackson wrote:

[snipp ubuntu explanation]

> I obviously thought it would be better to send you the patch we were
> actually using rather than some `special' version which we hadn't
> tested.  You are of course free to remove tests of this kind, but in
> the general that just makes a bit more work for the Ubuntu developer
> who next has to merge initramfs-tools from Debian.

curious on review :)
 
> Changes to debian/changelog always tend to get rejected if you apply a
> patch to a different version because the context is the previous
> version which contains the previous version number.

yeah it is a primitive versioning system.
 
 
> test(1) has really strange argument parsing, which is sometimes
> ambiguous.  Prefixing unknown input strings with x avoids accidentally
> treating those strings as operators or syntactic elements.
> 
> For example, consider:
>   test '(' '=' ')'
> 
> Is this
>   test '(' EXPR ')'   where  EXPR is '=', equivalent to  -n '='
> or
>   test A '=' B        where  A is '(' and B is ')'
> ?
> 
> It may be possible in particular cases to show that the problem cannot
> arise.  SuSv3 has a crazy set of explicit rules which only go as far
> as up to 4 arguments (and so answers my question above).  So both to
> avoid having to do a detailed analysis of test(1)'s argument parsing
> and of the possible input strings, and as a matter of good programming
> habit, it is better to write code the way it will always work (even if
> a subsequent programmer makes it more complex).

ok thanks for the lesson.
never the less i'd prefer to have more readable code,
then to take into account crazy non posix sh.
update-initramfs is anyway linux specific.

 
> In your version the "..." around `triggered' are superfluous.  When I
> see that in a shell script I start to wonder whether the author knows
> the proper sh quoting rules.  I trust that you do :-) but I would
> suggest avoiding idioms which tend to undermine the reader's
> confidence in the author.

ack
 
 
> > > +interest update-initramfs
> > "interest" ???
> 
> `interest' is the directive name used in the triggers file to specify
> that this package needs to know about activations of the named
> trigger.

ok, thanks i'll add your comment.
 
> I chose the explicit trigger name `update-initramfs' since it seemed
> like an obvious choice.  The guideance in the triggers spec (which I
> wrote, of course) says that it's often good to choose a command or
> package name and the command name seems like a good choice here.
> 
> Do you have a different view ?

no,
the meaning of the keywoard "interest" is new to me.
 
 
> > > +binary-install/initramfs-tools::
> > > +	install -m 644 -o 0 -g 0 debian/initramfs-tools.triggers \
> > > +		debian/initramfs-tools/DEBIAN/triggers
> > 
> > no i-t uses cdbs,
> > please add to debian/initramfs-tools.install
> > but maybe i misread and there is no cdbs support for that yet??
> 
> Stuffing files into DEBIAN/ with .install seemed a bit wrong to me,
> and I thought this approach with a hook was better.
> 
> In the future I imagine debhelper will copy <package>.triggers into
> the right place itself.

debhelper does it according to joeyh,
i haven't looked yet at how i'd have to invoke it.
 
 
> I agree that this isn't the best style.  One reason why I did it this
> way because I wanted to be sure that it did only and exactly what I
> intended it to.  Another way of looking at it is that I regarded
> myself as wrapping update-initramfs.  Obviously since update-initramfs
> is a script this could be done in code at the top of the script.

i see.
 
> > you may want to checkout latest git
> > git clone git://git.debian.org/git/kernel/initramfs-tools.git
> 
> Would you like me to prepare a new patch based on your comments ?

no need,
patch is explicit enough.

i'll see to push a variant in git next days,
may ping you for review..
so that i can upload it promptly with new dpkg.
 
amicalement
 
-- 
maks




Reply to: