Thank you for the extensive review. I went through your suggestions/comments and either fixed them or mentioned them here.
> I don't think the package descriptions should describe NetworkManager, > that is a job for the network-manager package description. Instead, > they should describe the network-manager-ssh packages. I know it's weird and somewhat unusual but all the other nm plugins do the same. So I went with it: http://anonscm.debian.org/cgit/pkg-utopia/network-manager-openvpn.git/tree/debian/control http://anonscm.debian.org/cgit/pkg-utopia/network-manager-pptp.git/tree/debian/control http://anonscm.debian.org/cgit/pkg-utopia/network-manager-vpnc.git/tree/debian/control > It would be great if there were a DEP-8 test: > > http://dep.debian.net/deps/dep8 > http://ci.debian.net This would definitely be a nice-to-have but it would require a decent test in the source package or writing one myself. As of now the test under test/ really doesn't do much. It basically just setups the env to actually start testing. So I'd say its more of a long term goal. > I always wonder if screenshots like images/*.png should be generated > at build time so they never get out of date. Screenshots don't change all that often and even if they are slightly out-of-date most people won't care. I'd say many just check the screenshots to check if its a GTK1 application last maintained in the 90s. > I wonder what "PCF" in the icon means, probably it would be better to > have "SSH" in there? The icon seems to be from nm-pptp. But even there it doesn't really make sense. So I guess the icon went x -> pptp -> openvpn -> ssh. > Upstream's po files look like they have poorly maintained or > unmaintained copyright headers. Most of them seem alright to me. A few have missing source package information but that information is available in debian/copyright anyhow. The translators seem to be attributed in all of them. > Upstream's test suite is using an IP address belonging to the USA > Department of Defence. I would strongly suggest using a proper private > IP address according to RFC 6761. > > http://tools.ietf.org/html/rfc6761 inetnum: 1.1.1.0 - 1.1.1.255 netname: APNIC-LABS descr: Research prefix for APNIC Labs address: South Brisbane, QLD 4101 address: Australia e-mail: ab...@apnic.net I know you are not supposed to use these. But the test is not executed and won't be unless upstream changes it as it is useless right now. > Please add some upstream metadata: > > https://wiki.debian.org/UpstreamMetadata Following the links on the page it seems to be mostly used by scientific libraries to reference algorithms used in the packages. All the basic repository/issue/copyright information is already present in other files in my current version. > Automated checks: > > check-all-the-things: > $ cppcheck -j1 --quiet -f . | grep -vF 'cppcheck: error: could not > find or open any of the paths given.' > [properties/nm-ssh.c:1287]: (error) Possible null pointer dereference: gateway > [properties/nm-ssh.c:1289]: (error) Possible null pointer dereference: > remote_ip > [properties/nm-ssh.c:1290]: (error) Possible null pointer dereference: > local_ip > [properties/nm-ssh.c:1291]: (error) Possible null pointer dereference: netmask > [properties/nm-ssh.c:1316]: (error) Possible null pointer dereference: > preferred_authentication > $ flawfinder -Q -c . > <lots> I'll report these upstream as these check seems to be of valid concern. Not security wise but to avoid errors. Thanks again, Lennart Weller
