Raphael Geissert <atomo64+deb...@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 (r...@debian.org) <http://www.eyrie.org/~eagle/>
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
