refactor to use Packages more and fix PartialEq,Ord impls on Package to all be consistent with each other

crates/pacdef/src/backend/actual/arch.rs

@@ -51,7 +51,7 @@ impl Backend for Arch {
     }
 
     /// Install the specified packages.
-    fn install_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn install_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new(&self.binary);
@@ -70,7 +70,7 @@ impl Backend for Arch {
     }
 
     /// Remove the specified packages.
-    fn remove_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn remove_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new(&self.binary);

crates/pacdef/src/backend/actual/debian.rs

@@ -54,7 +54,7 @@ impl Backend for Debian {
         Ok(result)
     }
 
-    fn make_dependency(&self, packages: &[Package]) -> Result<()> {
+    fn make_dependency(&self, packages: &Packages) -> Result<()> {
         let mut cmd = build_base_command_with_privileges("apt-mark");
         cmd.arg("auto");
         for p in packages {
@@ -65,7 +65,7 @@ impl Backend for Debian {
     }
 
     /// Install the specified packages.
-    fn install_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn install_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = build_base_command_with_privileges(&backend_info.binary);
@@ -84,7 +84,7 @@ impl Backend for Debian {
     }
 
     /// Remove the specified packages.
-    fn remove_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn remove_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = build_base_command_with_privileges(&backend_info.binary);

crates/pacdef/src/backend/actual/fedora.rs

@@ -74,7 +74,7 @@ impl Backend for Fedora {
     }
 
     /// Install the specified packages.
-    fn install_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn install_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new("sudo");
@@ -100,7 +100,7 @@ impl Backend for Fedora {
     }
 
     /// Show information from package manager for package.
-    fn remove_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn remove_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new("sudo");
@@ -128,7 +128,7 @@ impl Backend for Fedora {
         run_external_command(cmd)
     }
 
-    fn make_dependency(&self, _: &[Package]) -> Result<()> {
+    fn make_dependency(&self, _: &Packages) -> Result<()> {
         panic!("Not supported by the package manager!")
     }
 }

crates/pacdef/src/backend/actual/flatpak.rs

@@ -61,7 +61,7 @@ impl Backend for Flatpak {
     }
 
     /// Install the specified packages.
-    fn install_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn install_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new(backend_info.binary);
@@ -79,12 +79,12 @@ impl Backend for Flatpak {
         run_external_command(cmd)
     }
 
-    fn make_dependency(&self, _: &[Package]) -> Result<()> {
+    fn make_dependency(&self, _: &Packages) -> Result<()> {
         panic!("not supported by {}", self.backend_info().binary)
     }
 
     /// Remove the specified packages.
-    fn remove_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn remove_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new(backend_info.binary);

crates/pacdef/src/backend/actual/python.rs

@@ -72,7 +72,7 @@ impl Backend for Python {
         self.extract_packages(output)
     }
 
-    fn make_dependency(&self, _packages: &[Package]) -> Result<()> {
+    fn make_dependency(&self, _packages: &Packages) -> Result<()> {
         panic!("not supported by {}", self.binary)
     }
 }

crates/pacdef/src/backend/actual/rust.rs

@@ -55,7 +55,7 @@ impl Backend for Rust {
             .context("getting all installed packages")
     }
 
-    fn make_dependency(&self, _: &[Package]) -> Result<()> {
+    fn make_dependency(&self, _: &Packages) -> Result<()> {
         panic!("not supported by {}", self.backend_info().binary)
     }
 }

crates/pacdef/src/backend/actual/rustup/mod.rs

@@ -67,11 +67,11 @@ impl Backend for Rustup {
             .context("Getting all installed packages")
     }
 
-    fn make_dependency(&self, _: &[Package]) -> Result<()> {
+    fn make_dependency(&self, _: &Packages) -> Result<()> {
         panic!("Not supported by {}", self.backend_info().binary)
     }
 
-    fn install_packages(&self, packages: &[Package], _: bool) -> Result<()> {
+    fn install_packages(&self, packages: &Packages, _: bool) -> Result<()> {
         let packages = RustupPackage::from_pacdef_packages(packages)?;
 
         let (toolchains, components) =
@@ -83,7 +83,7 @@ impl Backend for Rustup {
         Ok(())
     }
 
-    fn remove_packages(&self, packages: &[Package], _: bool) -> Result<()> {
+    fn remove_packages(&self, packages: &Packages, _: bool) -> Result<()> {
         let rustup_packages = RustupPackage::from_pacdef_packages(packages)?;
 
         let (toolchains, components) =

crates/pacdef/src/backend/actual/rustup/types.rs

@@ -95,7 +95,7 @@ impl RustupPackage {
         (toolchains, components)
     }
 
-    pub fn from_pacdef_packages(packages: &[Package]) -> Result<Vec<Self>> {
+    pub fn from_pacdef_packages(packages: &Packages) -> Result<Vec<Self>> {
         let mut result = vec![];
 
         for package in packages {

crates/pacdef/src/backend/actual/void.rs

@@ -79,7 +79,7 @@ impl Backend for Void {
     }
 
     /// Install the specified packages.
-    fn install_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn install_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = build_base_command_with_privileges(INSTALL_BINARY);
@@ -96,7 +96,7 @@ impl Backend for Void {
         run_external_command(cmd)
     }
 
-    fn remove_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn remove_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = build_base_command_with_privileges(REMOVE_BINARY);
@@ -113,7 +113,7 @@ impl Backend for Void {
         run_external_command(cmd)
     }
 
-    fn make_dependency(&self, packages: &[Package]) -> Result<()> {
+    fn make_dependency(&self, packages: &Packages) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = build_base_command_with_privileges(PKGDB_BINARY);

crates/pacdef/src/backend/backend_trait.rs

@@ -1,6 +1,4 @@
-use std::cmp::{Eq, Ord};
-use std::collections::HashMap;
-use std::hash::Hash;
+use std::collections::BTreeMap;
 use std::process::Command;
 
 use anyhow::Result;
@@ -65,7 +63,12 @@ pub trait Backend {
     ///
     /// Returns an Error if any of the groups fails to save their given packages.
     fn assign_group(&self, to_assign: Vec<(Package, Group)>) -> Result<()> {
-        let group_package_map = to_hashmap(to_assign);
+        let mut group_package_map: BTreeMap<Group, Packages> = BTreeMap::new();
+
+        for (package, group) in to_assign {
+            group_package_map.entry(group).or_default().insert(package);
+        }
+
         let section_header = format!("[{}]", self.backend_info().section);
 
         for (group, packages) in group_package_map {
@@ -82,7 +85,7 @@ pub trait Backend {
     ///
     /// This function will return an error if the package manager cannot be run or it
     /// returns an error.
-    fn install_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn install_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new(self.backend_info().binary);
@@ -109,7 +112,7 @@ pub trait Backend {
     /// # Errors
     ///
     /// Returns an error if the external command fails.
-    fn make_dependency(&self, packages: &[Package]) -> Result<()> {
+    fn make_dependency(&self, packages: &Packages) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new(backend_info.binary);
@@ -130,7 +133,7 @@ pub trait Backend {
     /// # Errors
     ///
     /// Returns an error if the external command fails.
-    fn remove_packages(&self, packages: &[Package], noconfirm: bool) -> Result<()> {
+    fn remove_packages(&self, packages: &Packages, noconfirm: bool) -> Result<()> {
         let backend_info = self.backend_info();
 
         let mut cmd = Command::new(backend_info.binary);
@@ -162,24 +165,3 @@ pub trait Backend {
         run_external_command(cmd)
     }
 }
-
-/// For a vector of tuples containing a `V` and `K`, where a `K` may occur more than
-/// once and each `V` exactly once, create a `HashMap` that associates each `K` with
-/// a `Vec<V>`.
-fn to_hashmap<K, V>(to_assign: Vec<(V, K)>) -> HashMap<K, Vec<V>>
-where
-    K: Hash + Eq,
-    V: Ord,
-{
-    let mut map = HashMap::new();
-
-    for (value, key) in to_assign {
-        let inner: &mut Vec<V> = map.entry(key).or_default();
-        inner.push(value);
-    }
-
-    for vecs in map.values_mut() {
-        vecs.sort_unstable();
-    }
-    map
-}

crates/pacdef/src/backend/mod.rs

@@ -17,33 +17,35 @@ pub struct ManagedBackend {
 }
 
 impl ManagedBackend {
-    /// Get unmanaged packages, sorted alphabetically.
+    /// Get unmanaged packages
     ///
     /// # Errors
     ///
     /// Returns an error if the backend fails to get the explicitly installed packages.
-    pub fn get_unmanaged_packages_sorted(&self) -> Result<Vec<Package>> {
+    pub fn get_unmanaged_packages_sorted(&self) -> Result<Packages> {
         let installed = self
             .any_backend
             .get_explicitly_installed_packages()
             .context("could not get explicitly installed packages")?;
-        let mut diff: Vec<_> = installed.difference(&self.packages).cloned().collect();
-        diff.sort_unstable();
+
+        let diff = installed.difference(&self.packages).cloned().collect();
+
         Ok(diff)
     }
 
-    /// Get missing packages, sorted alphabetically.
+    /// Get missing packages
     ///
     /// # Errors
     ///
     /// Returns an error if the backend fails to get the installed packages.
-    pub fn get_missing_packages_sorted(&self) -> Result<Vec<Package>> {
+    pub fn get_missing_packages_sorted(&self) -> Result<Packages> {
         let installed = self
             .any_backend
             .get_all_installed_packages()
             .context("could not get installed packages")?;
-        let mut diff: Vec<_> = self.packages.difference(&installed).cloned().collect();
-        diff.sort_unstable();
+
+        let diff = self.packages.difference(&installed).cloned().collect();
+
         Ok(diff)
     }
 }

crates/pacdef/src/backend/todo_per_backend.rs

@@ -10,17 +10,17 @@ use crate::prelude::*;
 /// This struct is used to store a list of unmanaged packages or missing packages
 /// for all backends.
 #[derive(Debug)]
-pub struct ToDoPerBackend(Vec<(AnyBackend, Vec<Package>)>);
+pub struct ToDoPerBackend(Vec<(AnyBackend, Packages)>);
 impl ToDoPerBackend {
     pub fn new() -> Self {
         Self(vec![])
     }
 
-    pub fn push(&mut self, item: (AnyBackend, Vec<Package>)) {
+    pub fn push(&mut self, item: (AnyBackend, Packages)) {
         self.0.push(item);
     }
 
-    pub fn iter(&self) -> impl Iterator<Item = &(AnyBackend, Vec<Package>)> {
+    pub fn iter(&self) -> impl Iterator<Item = &(AnyBackend, Packages)> {
         self.0.iter()
     }
 
@@ -94,7 +94,7 @@ impl Default for ToDoPerBackend {
 }
 
 impl IntoIterator for ToDoPerBackend {
-    type Item = (AnyBackend, Vec<Package>);
+    type Item = (AnyBackend, Packages);
 
     type IntoIter = std::vec::IntoIter<Self::Item>;
 

crates/pacdef/src/grouping/group.rs

@@ -196,7 +196,7 @@ impl Group {
     ///
     /// This function returns an error if the group file cannot be read, or if the
     /// file cannot be written to.
-    pub fn save_packages(&self, section_header: &str, packages: &[Package]) -> Result<()> {
+    pub fn save_packages(&self, section_header: &str, packages: &Packages) -> Result<()> {
         let mut content = read_to_string(&self.path)
             .with_context(|| format!("reading existing file contents from {:?}", &self.path))?;
 
@@ -264,7 +264,7 @@ impl Display for Group {
 fn write_packages_to_existing_section(
     group_file_content: &mut String,
     section_header: &str,
-    packages: &[Package],
+    packages: &Packages,
 ) -> Result<()> {
     let idx_of_first_package_line_in_section =
         find_first_package_line_in_section(group_file_content, section_header)?;
@@ -306,7 +306,7 @@ fn find_first_package_line_in_section(
 fn add_new_section_with_packages(
     group_file_content: &mut String,
     section_header: &str,
-    packages: &[Package],
+    packages: &Packages,
 ) {
     group_file_content.push('\n');
     group_file_content.push_str(section_header);

crates/pacdef/src/grouping/package.rs

@@ -1,12 +1,12 @@
+use std::cmp::Ordering;
 use std::collections::BTreeSet;
 use std::fmt::{Display, Write};
-use std::hash::Hash;
 
 pub type Packages = BTreeSet<Package>;
 
 /// A struct to represent a single package, consisting of a `name`, and
 /// optionally a `repo`.
-#[derive(Debug, Eq, PartialOrd, Ord, Clone)]
+#[derive(Debug, Clone)]
 pub struct Package {
     /// The name of the package
     pub name: String,
@@ -54,7 +54,7 @@ impl Package {
     }
 
     /// Try to parse a string (from a line in a group file) and return a package.
-    /// From the string, any possible comment is removed and whitespace is trimmed.
+    /// From the string, any possible comment is removed and whitespace is trimmed.package
     /// Returns `None` if there is nothing left after trimming.
     pub fn try_from<S>(s: S) -> Option<Self>
     where
@@ -72,22 +72,25 @@ impl Package {
 
 impl PartialEq for Package {
     fn eq(&self, other: &Self) -> bool {
-        let self_repo = self.repo.as_ref();
-        let other_repo = other.repo.as_ref();
-
-        // iff both packages have repos, they must be identical, otherwise we don't care
-        let repos_are_identical =
-            self_repo.map_or(true, |sr| other_repo.map_or(true, |or| sr == or));
-
-        let names_are_identical = self.name == other.name;
-
-        names_are_identical && repos_are_identical
+        self.cmp(other).is_eq()
     }
 }
-
-impl Hash for Package {
-    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-        self.name.hash(state);
+impl Eq for Package {}
+impl PartialOrd for Package {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+impl Ord for Package {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.name
+            .cmp(&other.name)
+            .then(self.repo.as_ref().map_or(Ordering::Equal, |self_repo| {
+                other
+                    .repo
+                    .as_ref()
+                    .map_or(Ordering::Equal, |other_repo| self_repo.cmp(other_repo))
+            }))
     }
 }
 

crates/pacdef/src/review/datastructures.rs

@@ -48,9 +48,9 @@ impl ReviewsPerBackend {
         let mut result = vec![];
 
         for (backend, actions) in self {
-            let mut to_delete = vec![];
+            let mut to_delete = Packages::new();
             let mut assign_group = vec![];
-            let mut as_dependency = vec![];
+            let mut as_dependency = Packages::new();
 
             extract_actions(
                 actions,
@@ -91,15 +91,19 @@ pub enum ContinueWithReview {
 
 fn extract_actions(
     actions: Vec<ReviewAction>,
-    to_delete: &mut Vec<Package>,
+    to_delete: &mut Packages,
     assign_group: &mut Vec<(Package, Group)>,
-    as_dependency: &mut Vec<Package>,
+    as_dependency: &mut Packages,
 ) {
     for action in actions {
         match action {
-            ReviewAction::Delete(package) => to_delete.push(package),
+            ReviewAction::Delete(package) => {
+                to_delete.insert(package);
+            }
             ReviewAction::AssignGroup(package, group) => assign_group.push((package, group)),
-            ReviewAction::AsDependency(package) => as_dependency.push(package),
+            ReviewAction::AsDependency(package) => {
+                as_dependency.insert(package);
+            }
         }
     }
 }

crates/pacdef/src/review/strategy.rs

@@ -5,16 +5,16 @@ use crate::prelude::*;
 #[derive(Debug)]
 pub struct Strategy {
     backend: AnyBackend,
-    delete: Vec<Package>,
-    as_dependency: Vec<Package>,
+    delete: Packages,
+    as_dependency: Packages,
     assign_group: Vec<(Package, Group)>,
 }
 
 impl Strategy {
     pub fn new(
         backend: AnyBackend,
-        delete: Vec<Package>,
-        as_dependency: Vec<Package>,
+        delete: Packages,
+        as_dependency: Packages,
         assign_group: Vec<(Package, Group)>,
     ) -> Self {
         Self {