Fix behavior of install-n command
#1611
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for catching and fixing this! |
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in InstallCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `install-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf install-n joe-4.6 After this commit: $ dnf install-n joe-4.6 No match for argument: joe-4.6 * Maybe you meant: joe Error: Unable to find a match: joe-4.6
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in InstallCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `localinstall` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf localinstall joe After this commit: $ dnf localinstall joe Not a valid rpm file path: joe Error: Nothing to do.
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in AutoremoveCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `autoremove-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf autoremove-n joe-4.6 After this commit: $ dnf autoremove-n joe-4.6 No match for argument: joe-4.6 No packages marked for removal. Dependencies resolved. Nothing to do. Complete! This command still works as expected: $ dnf autoremove-n joe
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RemoveCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `remove-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf remove-n joe-4.6 After this commit: $ dnf remove-n joe-4.6 No match for argument: joe-4.6 No packages marked for removal. Dependencies resolved. Nothing to do. Complete! This command still works as expected: $ dnf remove-n joe
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RepoQueryCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `repoquery-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf repoquery-n joe-4.6 joe-0:4.6-6.fc31.x86_64 After this commit: $ dnf repoquery-n joe-4.6 (empty) This command still works as expected: $ dnf repoquery-n joe joe-0:4.6-6.fc31.x86_64
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in UpdateInfoCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `list-updateinfo` form and other aliases, since it would no longer detect that this alias was used and would use the default action (`summary`) instead of the one triggered by the alias (`list`). Before this commit, this command would incorrectly return the updateinfo summary only, instead of the full list: $ dnf list-updateinfo (same as `dnf updateinfo summary` output.) After this commit: $ dnf list-updateinfo (same as `dnf updateinfo list` output.)
filbranden
force-pushed
the
fix-install-n
branch
from
aa3fed5
to
6eca0bb
Compare
|
@m-blaha Great catch! I just went through the matches and fixed them one by one. I shipped those as separate commits, so that I could easily test them one by one in isolation. That should also make it easier to potentially backport them if necessary. (I think backporting to RHEL 8 dnf might make sense.) The last commit in the series (the one for the Thanks for the quick turnaround! Let me know if you'd like to see any further changes to this series, happy to oblige. Cheers, |
|
Thanks a lot for your patches! All the changes looks good to me so let's merge this. |
|
📌 Commit 6eca0bb has been approved by |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in InstallCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `install-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf install-n joe-4.6 After this commit: $ dnf install-n joe-4.6 No match for argument: joe-4.6 * Maybe you meant: joe Error: Unable to find a match: joe-4.6 Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in InstallCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `localinstall` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf localinstall joe After this commit: $ dnf localinstall joe Not a valid rpm file path: joe Error: Nothing to do. Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in AutoremoveCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `autoremove-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf autoremove-n joe-4.6 After this commit: $ dnf autoremove-n joe-4.6 No match for argument: joe-4.6 No packages marked for removal. Dependencies resolved. Nothing to do. Complete! This command still works as expected: $ dnf autoremove-n joe Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RemoveCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `remove-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf remove-n joe-4.6 After this commit: $ dnf remove-n joe-4.6 No match for argument: joe-4.6 No packages marked for removal. Dependencies resolved. Nothing to do. Complete! This command still works as expected: $ dnf remove-n joe Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RepoQueryCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `repoquery-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf repoquery-n joe-4.6 joe-0:4.6-6.fc31.x86_64 After this commit: $ dnf repoquery-n joe-4.6 (empty) This command still works as expected: $ dnf repoquery-n joe joe-0:4.6-6.fc31.x86_64 Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in UpdateInfoCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `list-updateinfo` form and other aliases, since it would no longer detect that this alias was used and would use the default action (`summary`) instead of the one triggered by the alias (`list`). Before this commit, this command would incorrectly return the updateinfo summary only, instead of the full list: $ dnf list-updateinfo (same as `dnf updateinfo summary` output.) After this commit: $ dnf list-updateinfo (same as `dnf updateinfo list` output.) Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RepoListCommand still handled it as if it was a list. It turns out this code didn't really break, since the Python `in` operator works on strings by checking the substring. So while this went from an exact match to a substring match, in this particular case the two will match the same cases. So `repoinfo` continued working as expected even after the list -> string change. Nevertheless, we should fix this code to now handle this properly as a string and use an exact comparison instead of the substring check. This commit also refactors some of the code, to use explicit `or` checks rather than an `any(...)` operator with a tuple, since there was no particular reason to use the `any(...)` form and the code didn't rely on not short-circuiting behavior of building the tuple (none of the conditions involved had side effects.) The commit also refactors two if statements that had similar sets of conditions to embed the first into the second. Tested that the `dnf repoinfo` command works the same before and after this change. Closes: #1611 Approved by: m-blaha
|
💔 Test failed - status-papr |
m-blaha
requested changes
Apr 1, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RepoListCommand still handled it as if it was a list. It turns out this code didn't really break, since the Python `in` operator works on strings by checking the substring. So while this went from an exact match to a substring match, in this particular case the two will match the same cases. So `repoinfo` continued working as expected even after the list -> string change. Nevertheless, we should fix this code to now handle this properly as a string and use an exact comparison instead of the substring check. This commit also refactors some of the code, to use explicit `or` checks rather than an `any(...)` operator with a tuple, since there was no particular reason to use the `any(...)` form and the code didn't rely on not short-circuiting behavior of building the tuple (none of the conditions involved had side effects.) Tested that the `dnf repoinfo` command works the same before and after this change. Also tested that `dnf repoinfo --all` keeps showing all repositories with their enabled/disabled status correctly.
filbranden
force-pushed
the
fix-install-n
branch
from
6eca0bb
to
5474820
Compare
|
📌 Commit 5474820 has been approved by |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 2, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in InstallCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `localinstall` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf localinstall joe After this commit: $ dnf localinstall joe Not a valid rpm file path: joe Error: Nothing to do. Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 2, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in AutoremoveCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `autoremove-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf autoremove-n joe-4.6 After this commit: $ dnf autoremove-n joe-4.6 No match for argument: joe-4.6 No packages marked for removal. Dependencies resolved. Nothing to do. Complete! This command still works as expected: $ dnf autoremove-n joe Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 2, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RemoveCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `remove-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf remove-n joe-4.6 After this commit: $ dnf remove-n joe-4.6 No match for argument: joe-4.6 No packages marked for removal. Dependencies resolved. Nothing to do. Complete! This command still works as expected: $ dnf remove-n joe Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 2, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RepoQueryCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `repoquery-n` form, since it would no longer detect that command to restrict search of packages. Before this commit, this command would (incorrectly) succeed: $ dnf repoquery-n joe-4.6 joe-0:4.6-6.fc31.x86_64 After this commit: $ dnf repoquery-n joe-4.6 (empty) This command still works as expected: $ dnf repoquery-n joe joe-0:4.6-6.fc31.x86_64 Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 2, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in UpdateInfoCommand still handled it as if it was a list. This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed. But it broke behavior of the `list-updateinfo` form and other aliases, since it would no longer detect that this alias was used and would use the default action (`summary`) instead of the one triggered by the alias (`list`). Before this commit, this command would incorrectly return the updateinfo summary only, instead of the full list: $ dnf list-updateinfo (same as `dnf updateinfo summary` output.) After this commit: $ dnf list-updateinfo (same as `dnf updateinfo list` output.) Closes: #1611 Approved by: m-blaha
rh-atomic-bot
pushed a commit
that referenced
this pull request
Apr 2, 2020
Commit de9643a (first shipped in dnf 4.2.8) changed opts.command from a list to a simple string, but code in RepoListCommand still handled it as if it was a list. It turns out this code didn't really break, since the Python `in` operator works on strings by checking the substring. So while this went from an exact match to a substring match, in this particular case the two will match the same cases. So `repoinfo` continued working as expected even after the list -> string change. Nevertheless, we should fix this code to now handle this properly as a string and use an exact comparison instead of the substring check. This commit also refactors some of the code, to use explicit `or` checks rather than an `any(...)` operator with a tuple, since there was no particular reason to use the `any(...)` form and the code didn't rely on not short-circuiting behavior of building the tuple (none of the conditions involved had side effects.) Tested that the `dnf repoinfo` command works the same before and after this change. Also tested that `dnf repoinfo --all` keeps showing all repositories with their enabled/disabled status correctly. Closes: #1611 Approved by: m-blaha
|
☀️ Test successful - status-papr |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit de9643a (first shipped in dnf 4.2.8) changed
opts.commandfrom a list to a simple string, but code inInstallCommandstill handledit as if it was a list.This broke silently because Python will iterate over a string character by character, so it never produced a syntax error or an exception that would be noticed.
But it broke behavior of the
install-nform, since it would no longer detect that command to restrict search of packages.Before this commit, this command would (incorrectly) succeed:
After this commit:
The latter is consistent with behavior in dnf 4.2.7 and earlier.