Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

avoid removing empty list of packages #1554

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+4 −2
Open

avoid removing empty list of packages #1554

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a85a119
check array length to avoid removing empty list of packages
lexming Sep 12, 2022
3f5fc56
also remove empty lines from output of LEAF_PACKAGES command
lexming Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ncm-spma/src/main/perl/spma/yum.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ sub packages_to_remove

# The leaf set doesn't contain the header lines, which are just
# garbage.
my $leaves = Set::Scalar->new(grep($_ !~ m{\s}, split(/\n/, $out)));
my $leaves = Set::Scalar->new(grep($_ !~ m{^$|\s}, split(/\n/, $out)));

my $candidates = $leaves-$wanted;

Expand Down Expand Up @@ -907,7 +907,9 @@ sub update_pkgs
$to_rm = $self->packages_to_remove($wanted);
defined($to_rm) or return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would do here

(defined($to_rm) && $to_rm) or return 0;

() is for readability; and untested ofcourse ;)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty $to_rm does not evaluate to false, so (defined($to_rm) && $to_rm) or return 0; continues execution.

Copy link
Author

@lexming lexming Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because an empty $to_rm is () and that is something evaluated as a string (just a guess)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird, i would have expected an empty Set::Scalar looking at the code. packages_to_remove shouldn't return an undef, so i'm probably missng something here why there is a defined in the first place...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the defined($to_rm) is intended as error handling in the specific case that the function returned undef, there's no reason for the rest of the code not to run if there is nothing to remove?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this very briefly, it looks like for some reason $to_rm is not a Set::Scalar here and it should be.

$self->spare_dependencies($to_rm, $to_install, $error_is_warn);
$tx = $self->schedule(REMOVE, $to_rm);
if (scalar(@$to_rm) > 0) {
$tx = $self->schedule(REMOVE, $to_rm);
}
}

$tx .= $self->schedule(INSTALL, $to_install);
Expand Down
Loading