-
Notifications
You must be signed in to change notification settings - Fork 56
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.
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
avoid removing empty list of packages #1554
base: main
Are you sure you want to change the base?
Conversation
@@ -907,7 +907,9 @@ sub update_pkgs | |||
$to_rm = $self->packages_to_remove($wanted); | |||
defined($to_rm) or return 0; |
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I found the actual underlying cause of this issue in our system. The output of the
The regex in
Each blank line becomes an element of Fix for the regex in f0c309b |
First change has been made, but I have a new comment
Thanks for finding the root cause, @lexming ! I think it's still worth leaving in the length check in addition the regex fix, but would you mind using |
@ned21 sure, commits squashed 👍 |
Looks like the tests are failing on:
|
Fixes #1553
disclaimer: I'm not an expert in Perl, so better solutions may exist.