Ubuntu
ntp package

Code review comment for ~kick-d/ubuntu/+source/ntp:logical/4.2.6.p5+dfsg-3ubuntu9

  1. Git
  2. lp:~kick-d/ubuntu/+source/ntp
  3. logical/4.2.6.p5+dfsg-3ubuntu9
  4. Merge into logical
Revision history for this message
Robie Basak (racb) wrote :

Hi Pierre,

Good job on this one. The only diff between this and 1:4.2.6.p5+dfsg-3ubuntu9 are debian/changelog and update-maintainer as I expect.

Good job also squashing down all the different Apparmor-related changes into one commit.

My only complaint is on commits 93ec5cb and 8c7e386. These both touch the same lines in debian/ntpdate.if-up. In 8c7e386 the "LOCKFILE" line is changed and "invoke-rc.d" lines added, and in 93ec5cb the "LOCKFILE" line is changed back again and one of the previously added "invoke-rc.d" lines is moved. This is churn that we want to squash out at this logical stage.

For example:

@@ -30,7 +30,7 @@ if [ -r /lib/udev/hotplug.functions ]; then
        wait_for_file /usr/sbin/ntpdate-debian
 fi

-LOCKFILE=/var/lock/ntpdate
+LOCKFILE=/var/lock/ntpdate-ifup

 # Avoid running more than one at a time
 if [ -x /usr/bin/lockfile-create ]; then

and then later:

@@ -30,7 +30,7 @@ if [ -r /lib/udev/hotplug.functions ]; then
        wait_for_file /usr/sbin/ntpdate-debian
 fi

-LOCKFILE=/var/lock/ntpdate-ifup
+LOCKFILE=/var/lock/ntpdate

 # Avoid running more than one at a time
 if [ -x /usr/bin/lockfile-create ]; then

Indicate churn to me. When figuring out the logical set of changes made, these hunks cancel out and so should not appear at all. The same applies to one of the two invoke-rc.d lines.

I would split "debian/ntpdate.if-up: Fix interaction with openntpd", which I take to be the "if [ -e /usr/sbin/openntpd ]; then" stanza into its own commit. Then there would be a separate commit that doesn't need to touch LOCKFILE and just adds the two invoke-rc.d lines directly into their final places. Since this distils the previous uploads, the commit messages will probably need tweaking a little so that they make sense.

Apart from this, everything else is byte-perfect. I'm not sure it's necessary to redo this. It's minor enough that I can probably just keep it in mind while reviewing your merge. So I'll do that next. No action needed here for now. Thanks!

« Back to merge proposal