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

Reply via email to