Bug#519268: lintian: same problem in mksh, but with a twist
Russ Allbery wrote:
> Raphael Geissert 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.
:)
A similar problem occurred when I tried to add recursive support to
checkbashisms.
>
> Sure, this is fine, but please update it for HEAD so that it can be
> applied without reverting the bug fix there.
>
Yeah, although it will make it a bit slower (theory; real impact is not big
deal) because it will need to iterate over all array instead of just
looking for hash keys.
>> 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.
Heh, it was a quick implementation a-la TDD; code cleanup follows later.
>
>> } 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.
I was thinking about a different approach: keeping %removed_diversions but
making each element an array, making the whole thing a hash of arrays of
hashes :)
>
>> + # find the widest regex:
>> + my @matches = grep {
>> + if ($wider =~ m/^$_$/) {
>> + $wider = $_;
>
> Buggy if $_ evaluates to false; you need a 1; here.
In what case would it evaluate to false? not that I'm against adding 1; :)
>
>> + # 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.
>
sure, that can be done in the code cleanup phase.
>> +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:
Yeah, I was going to rewrite that as an if structure.
>
> 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.
>
Sure
Will clean it up and polish it and re-send it as an mbox.
Cheers,
--
Raphael Geissert - Debian Maintainer
www.debian.org - get.debian.net
Reply to: