Add history rollback command and transaction merging #1558
Conversation
|
Tests update: rpm-software-management/ci-dnf-stack#1515 |
|
Basic functionality looks good! I'll do a code review tomorrow. |
| void run() override; | ||
|
|
||
| std::unique_ptr<TransactionSpecArguments> transaction_specs{nullptr}; |
There was a problem hiding this comment.
Consider making these private
| transaction_item_action_is_inbound(previous_replay->second.action)) || | ||
| (transaction_item_action_is_outbound(package_replay.action) && | ||
| transaction_item_action_is_outbound(previous_replay->second.action))) { | ||
| // When both actions for nevra are inbound (REINSTALL excluded) or outbound use newer one and log it/warninging? |
| insert_or_append(na_to_installed_nevras, name_arch, package_replay.nevra); | ||
| } else { | ||
| // Not installonly, keep only the latests action | ||
| nevra_to_package_replay.erase(na_to_installed_nevras[name_arch][0]); |
| transaction_item_action_to_string(previous_replay->second.action))); | ||
| nevra_to_package_replay[package_replay.nevra] = package_replay; | ||
| } else { | ||
| // Actions cancel out |
There was a problem hiding this comment.
This is true if both the current and previous replay are strictly either inbound or outbound, but does not necessarily hold if a replay is neither inbound nor outbound. I see that REASON_CHANGE and REINSTALL are handled above, but maybe it would be more defensive to explicitly check
(transaction_item_action_is_inbound(package_replay.action) &&
transaction_item_action_is_outbound(previous_replay->second.action)) ||
(transaction_item_action_is_outbound(package_replay.action) &&
transaction_item_action_is_inbound(previous_replay->second.action))
here and include an else branch for any other edge cases, for example, if some new TransactionItemAction is added.
There was a problem hiding this comment.
Sounds good, I will throw an exception in the new else block since it hard to foresee what could the new item action do.
|
|
||
| auto previous_replay = nevra_to_package_replay.find(package_replay.nevra); | ||
|
|
||
| // When the previous action is REASON_CHANGE or REINSTALL override it (act as if its not there), |
There was a problem hiding this comment.
Should this comment be on the else branch? Since this branch runs when the previous is neither REASON_CHANGE nor REINSTALL
There was a problem hiding this comment.
Possibly, I meant the comment as an explanation for the if condition.
I tried moving the comment to else branch now and to me it felt more confusing.
Though you are right, it is talking about when the else branch is taken.
| // the package isn't installed at the beginning other actions cannot be applied. | ||
| nevra_to_package_replay[package_replay.nevra] = package_replay; | ||
| if (package_replay.action != TransactionItemAction::INSTALL) { | ||
| // If some other version of this name_arch isn't removed in this transaction its an invalid transaction merge |
| namespace libdnf5::transaction { | ||
|
|
||
| // Merge a vector of transactions replays. | ||
| // Order matters when merging transacitons, we perfer the latest transaction actions (actions from TransactionReplays later in the vector). |
This will be used in transaction merging.
It will be used by goal in `resolve_reverted_transactions(...)`.
The order is: first revert all specified transactions and then merge them together.
|
Thanks! Since the CI is failing due to librepo 1.18 not being available, I will run the tests locally and merge when they finish. |
|
Tests passing! |
c4d5bbf
Merging is used to revert multiple transactions to achieve a rollback.
It includes extensive unit tests for merging various combinations of actions.