Bug#519268: lintian: same problem in mksh, but with a twist

[Russ Allbery <rra@debian.org>]

Raphael Geissert <atomo64+debian@gmail.com> writes:

> I think I addressed most¹ cases, please try the attached patch (note
> that it won't work with latest HEAD, as it needs more changes due to
> 297135b0; try with HEAD^).

Oh, I see what you're doing.  I didn't even think of that.

Sure, this is fine, but please update it for HEAD so that it can be
applied without reverting the bug fix there.

>  		if ($mode eq 'add') {
> -		    $added_diversions{$divert} = $file;
> -		    tag 'diversion-for-unknown-file', $divert, "$file:$."
> -			unless (exists $info->index->{$divert});
> +		    $added_diversions{$divert} = {'f'=>$file, 'l'=>$.};

Please don't use cryptic hash keys.  file and line (and removed later)
make the code more readable.

>  		} elsif ($mode eq 'remove') {
> -		    $removed_diversions{$divert} = [ $file, $. ];
> +		    $removed_diversions{$divert} = {'f'=>$file, 'l'=>$.};

This has to be an array to avoid the bug fixed in HEAD anyway, so I think
easier to leave this a list.

> +	# find the widest regex:
> +	my @matches = grep {
> +		    if ($wider =~ m/^$_$/) {
> +			$wider = $_;

Buggy if $_ evaluates to false; you need a 1; here.

> +	# replace all the occurences with the widest regex:
> +	for my $k (@matches) {
> +	    next if ($k eq $wider);
> +
> +	    if (exists($removed_diversions{$k})) {
> +		$removed_diversions{$wider} = $removed_diversions{$k};
> +		delete $removed_diversions{$k};
> +	    }

Needs to be reworked for an array of removals.

>      } elsif ($file eq 'postrm') {
>  	# Allow preinst and postinst to remove diversions the
>  	# package doesn't add to clean up after previous
>  	# versions of the package.
> +
> +	# remove all the backslashes added by quotemeta:
> +	$divert =~ s/\\//g;
> +
> +	# beautify the file name:
> +	if ($expand_diversions) {
> +	    $divert =~ s/\.\+/*/g;
> +	}

This should be a sub, since it's done in two places.

> +for my $divert (keys %added_diversions) {
> +    my $d = $added_diversions{$divert};

Less cryptic variables are better.

    my $attrs = $added_diversions{$divert};
    my $location = $attrs->{file} . ':' . $attrs->{line};

and then the resulting code is clearer.

> +    tag 'diversion-for-unknown-file', $divert, "$d->{'f'}:$d->{'l'}"
> +	unless ($expand_diversions || exists $info->index->{$divert});
> +
> +    tag 'diversion-for-unknown-file', $divert, "$d->{'f'}:$d->{'l'}"
> +	if ($expand_diversions
> +	    && not grep { $_ =~ m/$divertrx/ } keys %{$info->index});

Better to make it clearer that this is an either/or:

if ($expand_diversions) {
    tag 'diversion-for-unknown-file', $divert, $location
        unless exists $info->index->{$divert};
} else {
    tag 'diversion-for-unknown-file', $divert, $location
        unless grep { /$divertrx/ } keys %{ $info->index };
}

Looks good with those changes.

-- 
Russ Allbery (rra@debian.org)               <http://www.eyrie.org/~eagle/>