On 2014-09-30 15:21, Simon McVittie wrote:
> Thanks for reviewing; I hope I've fixed all your concerns.
>
Hi,
Thanks for amending your patch. Unfortunately, I have a couple of new
minor remarks. Furthermore, I have updated our internal API in some
cases that will be useful in this check as well, which I would like to
promote as well. :)
Though this time I have finished updating all Lintian checks, so now
they all serve as an example of how to use the new API (correctly).
> [...]
> +use Lintian::Util qw(slurp_entire_file);
Combined with a new feature, this import can be made redundant with a
proposed change later. See below.
> +
> +sub run {
> + my ($pkg, $type, $info) = @_;
> +
> + foreach my $file ($info->sorted_index) {
> + next if $file->is_dir;
> +
> + if ($file =~ m{^etc/dbus-1/(?:system|session)\.d/}) {
New feature: This (plus the outer loop) can now be replaced by
something like:
my @files;
for my $dirname qw(etc/dbus-1/session.d/ etc/dbus-1/system.d/) {
if (my $dir = $info->index_resolved_path($dirname)) {
push(@files, $dir->children);
}
}
for my $file (@files) {
next unless $file->is_open_ok;
_check_policy(...);
}
Which "only" iterates the two directories rather than the entire index
list. This is a fairly minor issue.
> + my $path = $info->index_resolved_path($file);
The call to index_resolved_path is redundant here; $file serves the same
purpose. There is only a "minor" difference with symlinks being
resolved - otherwise it is an expensive identity call[1]. But:
* $file->is_open_ok etc. are smart enough to resolve the symlink first.
* Based on your usage, the difference is redundant.
* If you really wanted the resolved path, then use $file->resolve_path
- NB: Can return undef for unsafe symlinks.
- It is cheaper for non-symlinks (than index_resolved_path).
NOTE: In git master, I have made $info->index_resolve_path($obj) error
out (where $obj is some sort of reference or e.g. a Lintian::Path object).
> + next unless $file->is_open_ok;
> + _check_policy($file, $path);
> + }
> + }
> +
> + return;
> +}
> +
> +sub _check_policy {
> + my ($file, $path) = @_;
> +
> + my $xml = slurp_entire_file($path->open);
New feature (making the import redundant as well):
my $xml = $file->file_contents;
NB: $path is redundant (see my argument above).
> +
> + [...]
Thanks for considering,
~Niels
[1] Admittedly, FSVO "expensive".
--
To UNSUBSCRIBE, email to debian-lint-maint-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: https://lists.debian.org/542c59be.4070...@thykier.net
