On 2015-06-28 03:09, Felipe Sateler wrote:
> Package: lintian
> Version: 2.5.32
> Severity: wishlist
> Tags: patch
>
> Hi,
>
> Please find attached a patch that does $subject. I have taken the
> liberty to refactor the code a bit in order to stop tagging multiple
> times for the same error.
>
> Patches 1-3 are the refactoring, patch 4 is the new check. There is a
> test for the new check.
>
Hi,
Thanks for working on this. I have a couple of remarks to some of the
patches (interleaved into the patches below).
> I'm wondering if tag systemd-no-service-for-init-script should be
> lowered in severity but added inconditionally... but that is a separate
> issue. If/when this patch is merged, I can provide a patch for changing
> that so we can discuss that.
>
Ok. :)
> [...]
>
> 0001-Reorder-systemd-checks.patch
>
>
>>From 1c4ad47fead2a6d32b5fdc6888ba7b5333804bcb Mon Sep 17 00:00:00 2001
> From: Felipe Sateler <fsate...@debian.org>
> Date: Sat, 27 Jun 2015 16:32:42 -0300
> Subject: [PATCH 1/4] Reorder systemd checks
>
> This reorder groups most checks inside the corresponding check_*
> ---
> checks/systemd.pm | 140
> ++++++++++++++++++++++++++++++------------------------
> 1 file changed, 79 insertions(+), 61 deletions(-)
>
> diff --git a/checks/systemd.pm b/checks/systemd.pm
> index d36cf65..4a45b49 100644
> --- a/checks/systemd.pm
> +++ b/checks/systemd.pm
> [...]
> +sub get_systemd_service_files {
> + my $info = shift @_;
Please use the "my ($info) = @_;" notation instead.
The use of "shift" is generally discouraged throughout the lintian code
base (I think we
> +
> + return grep { m,/systemd/system/.*\.service$, } $info->sorted_index;
> +}
> +
> +sub get_systemd_service_names {
> + my ($info) = @_;
> + my %services;
> +
> + [...]
> + return %services;
> +}
Please consider returning this as a ref. Since it is passed around, we
end up copying it several times. Admittedly, I suspect it is a small
hash, I am mostly in it for being consistent with similar usage for
larger hashes.
Quick cheat-sheet
return \%services;
$services{foo} => $services->{foo}
if (%foo) => if (%{$foo})
> +
> sub check_systemd_service_file {
> my ($info, $file) = @_;
>
> + tag 'systemd-service-file-outside-lib', $file if ($file =~
> m,^etc/systemd/system/,);
> + tag 'systemd-service-file-outside-lib', $file if ($file =~
> m,^usr/lib/systemd/system/,);
Note this will match non-files (including the etc/systemd/system
directory). If the systemd package ships that as an empty dir (or
containing a README).
Though presumably something already filters this out before we get that far?
> +
An "empty" whitespace line - please apply "perltidy -it=4 -b
checks/systemd.pm" (it will possibly reformat other things as well).
> my @values = extract_service_file_values($info, $file, 'Unit', 'After');
> my @obsolete = grep { /^(?:syslog|dbus)\.target$/ } @values;
> tag 'systemd-service-file-refers-to-obsolete-target', $file, $_
> @@ -236,13 +261,6 @@ sub extract_service_file_values {
> [...]
>
> 0002-Check-files-as-we-detect-them-and-discard-invalid-fi.patch
>
>
>>From f712f9246412799088f2893cb5323b8b9f295de3 Mon Sep 17 00:00:00 2001
> From: Felipe Sateler <fsate...@debian.org>
> Date: Sat, 27 Jun 2015 21:56:11 -0300
> Subject: [PATCH 2/4] Check files as we detect them, and discard invalid files
>
> prevents duplicate service-file-is-not-a-file
\o/
> ---
> checks/systemd.pm | 32 +++++++++++++++++---------------
> t/tests/systemd-general/tags | 1 -
> 2 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/checks/systemd.pm b/checks/systemd.pm
> index 4a45b49..2fd2c82 100644
> --- a/checks/systemd.pm
> +++ b/checks/systemd.pm
> [...]
> @@ -124,12 +120,18 @@ sub check_init_script {
> [...]
> sub get_systemd_service_names {
> - my ($info) = @_;
> + my $info = shift @_;
> + my @files = @_;
Again, please prefer:
my ($info, @files) = @_;
Alternatively, consider making @files a ref. Again, not a high priority
since I doubt any package will ever ship a "significant" amount of
service files.
If you do, the cheat sheet is:
get_systemd_service_names($info, @files) =>
get_systemd_service_names($info, \@files)
my ($info, $files_ref) = @_;
@files => @{$files_ref}
> my %services;
>
> my $safe_add_service = sub {
> @@ -141,7 +143,7 @@ sub get_systemd_service_names {
> [...]
>>From f98b16ffd7c8adb603fa6de4afc9dfc06c142764 Mon Sep 17 00:00:00 2001
> From: Felipe Sateler <fsate...@debian.org>
> Date: Sat, 27 Jun 2015 22:01:19 -0300
> Subject: [PATCH 3/4] Add parameter to prevent tagging when parsing values
>
> Enables us to prevent multiple service-key-has-whitespace
> [...]
\o/
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
