Code review comment for lp://staging/~xnox/ubuntu/quantal/sudo/merge

> - debian/patches/CVE-2012-0809.patch:
> + dropped, included in this new upstream release.
> - debian/patches/enable_badpass.patch:
> + dropped as Debian chose to set this by default in the sudoers.
>
> Please document these separately in debian/changelog, not as part of
> "remaining changes" since these are changes that *don't* remain. ("Dropped
> changes" makes a good header for such things.)
>

ok.

> debian/sudoers has been changed - this means that debian/sudo.preinst also
> needs updated, for avoid_conffile_prompt() to know about the new checksums.
> The version check also needs updated from 1.8.3p1-1ubuntu1 to
> 1.8.3p2-1ubuntu1.
>

Why? Last time around our preinst was 'upgrading' to a config file with a non-matching checksum, which was then resulting in the prompt. This has been fixed in the last upload. So currently /etc/sudoers is managed as a dpkg conffile with correct checksums and gets auto updated to a new version if it is untouched.

Upgrading Lucid's sudo to the new merged version asks no questions:
(Reading database ... 15827 files and directories currently installed.)
Preparing to replace sudo 1.8.3p1-1ubuntu3.1 (using .../sudo_1.8.3p2-0~56~precise1_amd64.deb) ...
invoke-rc.d: policy-rc.d denied execution of stop.
Unpacking replacement sudo ...
Setting up sudo (1.8.3p2-0~56~precise1) ...
Installing new version of config file /etc/sudoers ...
Installing new version of config file /etc/sudoers.d/README ...
Installing new version of config file /etc/init.d/sudo ...

I have left the logic there just for someone who is jumping releases on upgrade (e.g. lucid -> quantal or oneiric -> quantal).
Potentially this is cruft, since an LTS got released & we should be removing this duplication of the same config file in two places.

> debian/patches/paths-in-samples.diff, debian/patches/typo-in-classic-
> insults.diff have modified headers relative to the Debian package when they
> don't need to... this cosmetic delta could be dropped.
>

ok. sorry.
I refreshed them due to fuzz, but dpkg-source seems to unpack this little fuzz ok this time around.

> debian/rules passes --enable-admin-flag to the main sudo build, but not to the
> ldap build - I think this is a bug? (Not one introduced by your merge, but
> one that should be fixed nevertheless)
>

I agree that it's a bug.
Also sudo_root 8 man page was not installed in the sudo-ldap package on ubuntu.
People seem to be renaming Vcs-* control fields to XS-Debian-Vcs-*; not sure what the consensus is behind this, i.e. is to make debcheckout work on merged packages?!

> Otherwise, this looks good to me.

Thanks.