replace raw HashMap with Storages

README.md

@@ -5,6 +5,7 @@
   - [ ] with ssh credential
     - [ ] ssh-agent
     - [ ] specify key
+- [ ] add storage remove command
 - [ ] add sync subcommand
 - [ ] add check subcommand
 - [ ] reorganize cmd option for storage

src/cmd_storage.rs

@@ -3,7 +3,8 @@
 use std::{
     collections::HashMap,
     io::{self, Write},
-    path::{Path, PathBuf}, string,
+    path::{Path, PathBuf},
+    string,
 };
 
 use anyhow::{anyhow, Context, Result};
@@ -14,12 +15,13 @@ use inquire::{min_length, Confirm, CustomType, Select, Text};
 use unicode_width::{self, UnicodeWidthStr};
 
 use crate::{
-    add_and_commit, ask_unique_name,
+    add_and_commit,
     cmd_args::Cli,
     devices::{self, Device},
     inquire_filepath_completer::FilePathCompleter,
     storages::{
         self, directory, local_info, physical_drive_partition, Storage, StorageExt, StorageType,
+        Storages,
     },
 };
 
@@ -32,12 +34,12 @@ pub(crate) fn cmd_storage_add(
     trace!("Storage Add {:?}, {:?}", storage_type, path);
     // Get storages
     // let mut storages: Vec<Storage> = get_storages(&config_dir)?;
-    let mut storages: HashMap<String, Storage> = storages::get_storages(&config_dir)?;
+    let mut storages = Storages::read(&config_dir)?;
     trace!("found storages: {:?}", storages);
 
     let device = devices::get_device(&config_dir)?;
     let (key, storage) = match storage_type {
-        StorageType::Physical => {
+        StorageType::P => {
             let use_sysinfo = {
                 let options = vec![
                     "Fetch disk information automatically.",
@@ -64,7 +66,7 @@ pub(crate) fn cmd_storage_add(
                         .with_validator(min_length!(0, "At least 1 character"))
                         .prompt()
                         .context("Failed to get Name")?;
-                    if storages.iter().all(|(k, _v)| k != &name) {
+                    if storages.list.iter().all(|(k, _v)| k != &name) {
                         break;
                     }
                     println!("The name {} is already used.", name);
@@ -104,8 +106,8 @@ pub(crate) fn cmd_storage_add(
             println!("storage: {}: {:?}", key, storage);
             (key, Storage::PhysicalStorage(storage))
         }
-        StorageType::SubDirectory => {
+        StorageType::S => {
-            if storages.is_empty() {
+            if storages.list.is_empty() {
                 return Err(anyhow!(
                     "No storages found. Please add at least 1 physical storage first."
                 ));
@@ -135,7 +137,7 @@ pub(crate) fn cmd_storage_add(
             )?;
             (key_name, Storage::SubDirectory(storage))
         }
-        StorageType::Online => {
+        StorageType::O => {
             let path = path.unwrap_or_else(|| {
                 let mut cmd = Cli::command();
                 cmd.error(
@@ -150,7 +152,7 @@ pub(crate) fn cmd_storage_add(
                     .with_validator(min_length!(0, "At least 1 character"))
                     .prompt()
                     .context("Failed to get Name")?;
-                if storages.iter().all(|(k, _v)| k != &name) {
+                if storages.list.iter().all(|(k, _v)| k != &name) {
                     break;
                 }
                 println!("The name {} is already used.", name);
@@ -178,11 +180,11 @@ pub(crate) fn cmd_storage_add(
     };
 
     // add to storages
-    storages.insert(key.clone(), storage);
+    storages.add(storage)?;
     trace!("updated storages: {:?}", storages);
 
     // write to file
-    storages::write_storages(&config_dir, storages)?;
+    storages.write(&config_dir)?;
 
     // commit
     add_and_commit(
@@ -198,7 +200,7 @@ pub(crate) fn cmd_storage_add(
 
 pub(crate) fn cmd_storage_list(config_dir: &PathBuf, with_note: bool) -> Result<()> {
     // Get storages
-    let storages: HashMap<String, Storage> = storages::get_storages(&config_dir)?;
+    let storages = Storages::read(&config_dir)?;
     trace!("found storages: {:?}", storages);
     let device = devices::get_device(&config_dir)?;
     let mut stdout = io::BufWriter::new(io::stdout());
@@ -209,17 +211,18 @@ pub(crate) fn cmd_storage_list(config_dir: &PathBuf, with_note: bool) -> Result<
 
 fn write_storages_list(
     mut writer: impl io::Write,
-    storages: &HashMap<String, Storage>,
+    storages: &Storages,
     device: &Device,
     long_display: bool,
 ) -> Result<()> {
     let name_width = storages
+        .list
         .iter()
         .map(|(_k, v)| v.name().width())
         .max()
         .unwrap();
     trace!("name widths: {}", name_width);
-    for (_k, storage) in storages {
+    for (_k, storage) in &storages.list {
         let size_str = match storage.capacity() {
             Some(b) => Byte::from_bytes(b.into())
                 .get_appropriate_unit(true)
@@ -244,7 +247,7 @@ fn write_storages_list(
             |v| v.display().to_string(),
         );
         let parent_name = if let Storage::SubDirectory(s) = storage {
-            s.parent(&storages)
+            s.parent(&storages)?
                 .context(format!("Failed to get parent of storage {}", s))?
                 .name()
         } else {
@@ -262,21 +265,11 @@ fn write_storages_list(
         )?;
         if long_display {
             let note = match storage {
-                Storage::PhysicalStorage(s) => {
+                Storage::PhysicalStorage(s) => s.kind(),
-                    s.kind()
+                Storage::SubDirectory(s) => &s.notes,
-                },
+                Storage::Online(s) => &s.provider,
-                Storage::SubDirectory(s) => {
-                    &s.notes
-                },
-                Storage::Online(s) => {
-                    &s.provider
-                },
             };
-            writeln!(
+            writeln!(writer, "    {}", note)?;
-                writer,
-                "    {}",
-                note
-            )?;
         }
     }
     Ok(())
@@ -291,10 +284,11 @@ pub(crate) fn cmd_storage_bind(
 ) -> Result<()> {
     let device = devices::get_device(&config_dir)?;
     // get storages
-    let mut storages: HashMap<String, Storage> = storages::get_storages(&config_dir)?;
+    let mut storages = Storages::read(&config_dir)?;
     let commit_comment = {
         // find matching storage
         let storage = &mut storages
+            .list
             .get_mut(&storage_name)
             .context(format!("No storage has name {}", storage_name))?;
         let old_alias = storage
@@ -310,7 +304,7 @@ pub(crate) fn cmd_storage_bind(
     trace!("bound new system name to the storage");
     trace!("storages: {:#?}", storages);
 
-    storages::write_storages(&config_dir, storages)?;
+    storages.write(&config_dir)?;
     // commit
     add_and_commit(
         &repo,
@@ -326,3 +320,15 @@ pub(crate) fn cmd_storage_bind(
     );
     Ok(())
 }
+
+fn ask_unique_name(storages: &Storages, target: String) -> Result<String> {
+    let mut disk_name = String::new();
+    loop {
+        disk_name = Text::new(format!("Name for {}:", target).as_str()).prompt()?;
+        if storages.list.iter().all(|(k, v)| k != &disk_name) {
+            break;
+        }
+        println!("The name {} is already used.", disk_name);
+    }
+    Ok(disk_name)
+}

src/main.rs

@@ -18,6 +18,7 @@ use git2::{Commit, Oid, Repository};
 use inquire::{min_length, Confirm, CustomType, Select};
 use inquire::{validator::Validation, Text};
 use serde_yaml;
+use storages::Storages;
 use std::collections::HashMap;
 use std::path::Path;
 use std::path::{self, PathBuf};
@@ -25,7 +26,7 @@ use std::path::{self, PathBuf};
 use crate::cmd_args::{Cli, Commands, StorageCommands};
 use crate::storages::online_storage;
 use crate::storages::{
-    directory, get_storages, local_info, physical_drive_partition, write_storages, Storage,
+    directory, local_info, physical_drive_partition, Storage,
     StorageExt, StorageType, STORAGESFILE,
 };
 use devices::{Device, DEVICESFILE, *};
@@ -97,7 +98,7 @@ fn main() -> Result<()> {
         Commands::Check {} => {
             println!("Config dir: {}", &config_dir.display());
             let _storages =
-                storages::get_storages(&config_dir).context("Failed to parse storages file.");
+                Storages::read(&config_dir)?;
             todo!()
         }
     }
@@ -105,18 +106,6 @@ fn main() -> Result<()> {
     Ok(())
 }
 
-fn ask_unique_name(storages: &HashMap<String, Storage>, target: String) -> Result<String> {
-    let mut disk_name = String::new();
-    loop {
-        disk_name = Text::new(format!("Name for {}:", target).as_str()).prompt()?;
-        if storages.iter().all(|(k, v)| k != &disk_name) {
-            break;
-        }
-        println!("The name {} is already used.", disk_name);
-    }
-    Ok(disk_name)
-}
-
 fn find_last_commit(repo: &Repository) -> Result<Option<Commit>, git2::Error> {
     if repo.is_empty()? {
         Ok(None)

src/storages.rs

@@ -1,22 +1,32 @@
 //! Manipulates storages.
 
-use crate::devices;
 use crate::storages::directory::Directory;
 use crate::storages::online_storage::OnlineStorage;
 use crate::storages::physical_drive_partition::PhysicalDrivePartition;
+use crate::{devices, storages};
 use anyhow::{anyhow, Context, Result};
 use clap::ValueEnum;
+use core::panic;
 use serde::{Deserialize, Serialize};
-use std::{collections::HashMap, ffi, fmt, fs, io, path, u64};
+use std::ffi::OsString;
+use std::{
+    collections::HashMap,
+    ffi,
+    fmt::{self, format},
+    fs, io, path, u64,
+};
 
 /// YAML file to store known storages..
 pub const STORAGESFILE: &str = "storages.yml";
 
 #[derive(ValueEnum, Clone, Copy, Debug)]
 pub enum StorageType {
-    Physical,
+    /// Physical storage
-    SubDirectory,
+    P,
-    Online,
+    /// Sub directory
+    S,
+    /// Online storage
+    O,
 }
 
 /// All storage types.
@@ -67,7 +77,7 @@ impl StorageExt for Storage {
     fn mount_path(
         &self,
         device: &devices::Device,
-        storages: &HashMap<String, Storage>,
+        storages: &Storages,
     ) -> Result<path::PathBuf> {
         match self {
             Self::PhysicalStorage(s) => s.mount_path(&device, &storages),
@@ -96,6 +106,14 @@ impl StorageExt for Storage {
             Storage::Online(s) => s.capacity(),
         }
     }
+
+    fn parent<'a>(&'a self, storages: &'a Storages) -> Result<Option<&Storage>> {
+        match self {
+            Storage::PhysicalStorage(s) => s.parent(storages),
+            Storage::SubDirectory(s) => s.parent(storages),
+            Storage::Online(s) => s.parent(storages),
+        }
+    }
 }
 
 impl fmt::Display for Storage {
@@ -130,7 +148,7 @@ pub trait StorageExt {
     fn mount_path(
         &self,
         device: &devices::Device,
-        storages: &HashMap<String, Storage>,
+        storages: &Storages,
     ) -> Result<path::PathBuf>;
 
     /// Add local info of `device` to `self`.
@@ -140,6 +158,9 @@ pub trait StorageExt {
         mount_point: path::PathBuf,
         device: &devices::Device,
     ) -> Result<()>;
+
+    /// Get parent
+    fn parent<'a>(&'a self, storages: &'a Storages) -> Result<Option<&Storage>>;
 }
 
 pub mod directory;
@@ -147,36 +168,111 @@ pub mod local_info;
 pub mod online_storage;
 pub mod physical_drive_partition;
 
-/// Get `HashMap<String, Storage>` from devices.yml([devices::DEVICESFILE]).
+#[derive(Debug, Serialize, Deserialize)]
-/// If [devices::DEVICESFILE] isn't found, return empty vec.
+pub struct Storages {
-pub fn get_storages(config_dir: &path::Path) -> Result<HashMap<String, Storage>> {
+    pub list: HashMap<String, Storage>,
-    if let Some(storages_file) = fs::read_dir(&config_dir)?
+}
-        .filter(|f| {
+
-            f.as_ref().map_or_else(
+impl Storages {
-                |_e| false,
+    /// Construct empty [`Storages`]
-                |f| {
+    pub fn new() -> Storages {
-                    let storagesfile: ffi::OsString = STORAGESFILE.into();
+        Storages {
-                    f.path().file_name() == Some(&storagesfile)
+            list: HashMap::new(),
-                },
+        }
-            )
+    }
-        })
+
-        .next()
+    /// Get [`Storage`] with `name`.
-    {
+    pub fn get(&self, name: &String) -> Option<&Storage> {
-        trace!("{} found: {:?}", STORAGESFILE, storages_file);
+        self.list.get(name)
-        let f = fs::File::open(config_dir.join(STORAGESFILE))?;
+    }
+
+    /// Add new [`Storage`] to [`Storages`]
+    /// New `storage` must has new unique name.
+    pub fn add(&mut self, storage: Storage) -> Result<()> {
+        if self.list.keys().any(|name| name == storage.name()) {
+            return Err(anyhow!(format!(
+                "Storage name {} already used",
+                storage.name()
+            )));
+        }
+        match self.list.insert(storage.name().to_string(), storage) {
+            Some(v) => {
+                error!("Inserted storage with existing name: {}", v);
+                panic!("unexpected behavior")
+            }
+            None => Ok(()),
+        }
+    }
+
+    /// Remove `storage` from [`Storages`].
+    /// Returns `Result` of removed [`Storage`].
+    pub fn remove(mut self, storage: Storage) -> Result<Option<Storage>> {
+        // dependency check
+        if self.list.iter().any(|(_k, v)| {
+            v.parent(&self)
+                .unwrap()
+                .is_some_and(|parent| parent.name() == storage.name())
+        }) {
+            return Err(anyhow!(
+                "Dependency error: storage {} has some children",
+                storage.name()
+            ));
+        }
+        Ok(self.list.remove(storage.name()))
+    }
+
+    /// Load [`Storages`] from data in `config_dir`.
+    pub fn read(config_dir: &path::Path) -> Result<Self> {
+        let storages_file = config_dir.join(STORAGESFILE);
+        if !storages_file.exists() {
+            trace!("No storages file found. Returning new `Storages` object.");
+            return Ok(Storages::new());
+        }
+        trace!("Reading {:?}", storages_file);
+        let f = fs::File::open(storages_file)?;
         let reader = io::BufReader::new(f);
-        let yaml: HashMap<String, Storage> =
+        let yaml: Storages =
-            serde_yaml::from_reader(reader).context("Failed to read devices.yml")?;
+            serde_yaml::from_reader(reader).context("Failed to parse storages.yml")?;
         Ok(yaml)
-    } else {
+    }
-        trace!("No {} found", STORAGESFILE);
+
-        Ok(HashMap::new())
+    pub fn write(self, config_dir: &path::Path) -> Result<()> {
+        let f = fs::File::create(config_dir.join(STORAGESFILE)).context("Failed to open storages file")?;
+        let writer = io::BufWriter::new(f);
+        serde_yaml::to_writer(writer, &self).context(format!("Failed to writing to {:?}", STORAGESFILE))
     }
 }
 
-/// Write `storages` to yaml file in `config_dir`.
+// /// Get `HashMap<String, Storage>` from devices.yml([devices::DEVICESFILE]).
-pub fn write_storages(config_dir: &path::Path, storages: HashMap<String, Storage>) -> Result<()> {
+// /// If [devices::DEVICESFILE] isn't found, return empty vec.
-    let f = fs::File::create(config_dir.join(STORAGESFILE))?;
+// pub fn get_storages(config_dir: &path::Path) -> Result<HashMap<String, Storage>> {
-    let writer = io::BufWriter::new(f);
+//     if let Some(storages_file) = fs::read_dir(&config_dir)?
-    serde_yaml::to_writer(writer, &storages).map_err(|e| anyhow!(e))
+//         .filter(|f| {
-}
+//             f.as_ref().map_or_else(
+//                 |_e| false,
+//                 |f| {
+//                     let storagesfile: ffi::OsString = STORAGESFILE.into();
+//                     f.path().file_name() == Some(&storagesfile)
+//                 },
+//             )
+//         })
+//         .next()
+//     {
+//         trace!("{} found: {:?}", STORAGESFILE, storages_file);
+//         let f = fs::File::open(config_dir.join(STORAGESFILE))?;
+//         let reader = io::BufReader::new(f);
+//         let yaml: HashMap<String, Storage> =
+//             serde_yaml::from_reader(reader).context("Failed to read devices.yml")?;
+//         Ok(yaml)
+//     } else {
+//         trace!("No {} found", STORAGESFILE);
+//         Ok(HashMap::new())
+//     }
+// }
+//
+// /// Write `storages` to yaml file in `config_dir`.
+// pub fn write_storages(config_dir: &path::Path, storages: HashMap<String, Storage>) -> Result<()> {
+//     let f = fs::File::create(config_dir.join(STORAGESFILE))?;
+//     let writer = io::BufWriter::new(f);
+//     serde_yaml::to_writer(writer, &storages).map_err(|e| anyhow!(e))
+// }

src/storages/directory.rs

@@ -2,11 +2,15 @@
 
 use anyhow::{anyhow, Context, Result};
 use serde::{Deserialize, Serialize};
-use std::{collections::HashMap, fmt, path};
+use std::{
+    collections::HashMap,
+    fmt::{self, format},
+    path,
+};
 
 use crate::devices;
 
-use super::{local_info::LocalInfo, Storage, StorageExt};
+use super::{local_info::LocalInfo, Storage, StorageExt, Storages};
 
 /// Subdirectory of other [Storage]s.
 #[derive(Serialize, Deserialize, Debug)]
@@ -44,9 +48,10 @@ impl Directory {
         path: path::PathBuf,
         notes: String,
         device: &devices::Device,
-        storages: &HashMap<String, Storage>,
+        storages: &Storages,
     ) -> Result<Directory> {
         let (parent_name, diff_path) = storages
+            .list
             .iter()
             .filter(|(_k, v)| v.has_alias(&device))
             .filter_map(|(k, v)| {
@@ -84,24 +89,12 @@ impl Directory {
         )
     }
 
-    /// Get parent `&Storage` of directory.
-    pub fn parent<'a>(&'a self, storages: &'a HashMap<String, Storage>) -> Result<&Storage> {
-        let parent = &self.parent;
-        storages.get(parent).context(format!(
-            "No parent {} exists for directory {}",
-            parent,
-            self.name()
-        ))
-    }
-
     /// Resolve mount path of directory with current device.
-    fn mount_path(
+    fn mount_path(&self, device: &devices::Device, storages: &Storages) -> Result<path::PathBuf> {
-        &self,
+        let parent_mount_path = self
-        device: &devices::Device,
+            .parent(&storages)?
-        storages: &HashMap<String, Storage>,
+            .context("Can't find parent storage")?
-    ) -> Result<path::PathBuf> {
+            .mount_path(&device, &storages)?;
-        let parent = self.parent(&storages)?;
-        let parent_mount_path = parent.mount_path(&device, &storages)?;
         Ok(parent_mount_path.join(self.relative_path.clone()))
     }
 }
@@ -122,7 +115,7 @@ impl StorageExt for Directory {
     fn mount_path(
         &self,
         device: &devices::Device,
-        storages: &HashMap<String, Storage>,
+        storages: &Storages,
     ) -> Result<path::PathBuf> {
         Ok(self.mount_path(device, storages)?)
     }
@@ -141,6 +134,17 @@ impl StorageExt for Directory {
         };
         Ok(())
     }
+
+    // Get parent `&Storage` of directory.
+    fn parent<'a>(&'a self, storages: &'a Storages) -> Result<Option<&Storage>> {
+        match storages.get(&self.parent).context(format!(
+            "No parent {} exists for directory {}",
+            &self.parent, &self.name
+        )) {
+            Ok(s) => Ok(Some(s)),
+            Err(e) => Err(anyhow!(e)),
+        }
+    }
 }
 
 impl fmt::Display for Directory {
@@ -164,7 +168,7 @@ mod test {
         devices::Device,
         storages::{
             self, local_info::LocalInfo, physical_drive_partition::PhysicalDrivePartition, Storage,
-            StorageExt,
+            StorageExt, Storages,
         },
     };
 
@@ -195,20 +199,14 @@ mod test {
             "some note".to_string(),
             local_infos,
         );
-        let mut storages: HashMap<String, Storage> = HashMap::new();
+        let mut storages = Storages::new();
-        storages.insert(
+        storages.add(storages::Storage::PhysicalStorage(physical)).unwrap();
-            physical.name().to_string(),
+        storages.add(Storage::SubDirectory(directory)).unwrap();
-            Storage::PhysicalStorage(physical),
-        );
-        storages.insert(
-            directory.name().to_string(),
-            Storage::SubDirectory(directory),
-        );
         // assert_eq!(directory.name(), "test_name");
-        assert_eq!(storages.get("test_name").unwrap().name(), "test_name");
+        assert_eq!(storages.get(&"test_name".to_string()).unwrap().name(), "test_name");
         assert_eq!(
             storages
-                .get("test_name")
+                .get(&"test_name".to_string())
                 .unwrap()
                 .mount_path(&device, &storages)
                 .unwrap(),

src/storages/online_storage.rs

@@ -11,7 +11,7 @@ use crate::devices;
 
 use super::{
     local_info::{self, LocalInfo},
-    StorageExt,
+    StorageExt, Storages,
 };
 
 #[derive(Serialize, Deserialize, Debug)]
@@ -57,7 +57,7 @@ impl StorageExt for OnlineStorage {
     fn mount_path(
         &self,
         device: &devices::Device,
-        _storages: &HashMap<String, super::Storage>,
+        _storages: &Storages,
     ) -> Result<std::path::PathBuf> {
         Ok(self
             .local_infos
@@ -81,6 +81,13 @@ impl StorageExt for OnlineStorage {
         };
         Ok(())
     }
+
+    fn parent(
+        &self,
+        storages: &Storages,
+    ) -> Result<Option<&super::Storage>> {
+        Ok(None)
+    }
 }
 
 impl fmt::Display for OnlineStorage {

src/storages/physical_drive_partition.rs

@@ -2,7 +2,7 @@
 
 use crate::devices;
 use crate::devices::Device;
-use crate::storages::{Storage, StorageExt};
+use crate::storages::{Storage, StorageExt, Storages};
 use anyhow::{anyhow, Context, Result};
 use byte_unit::Byte;
 use inquire::Text;
@@ -106,7 +106,10 @@ impl PhysicalDrivePartition {
 
 #[cfg(test)]
 mod test {
-    use crate::{devices::Device, storages::{local_info::LocalInfo, StorageExt}};
+    use crate::{
+        devices::Device,
+        storages::{local_info::LocalInfo, StorageExt},
+    };
     use std::path::PathBuf;
 
     use super::PhysicalDrivePartition;
@@ -141,11 +144,7 @@ impl StorageExt for PhysicalDrivePartition {
         self.local_infos.get(&device.name())
     }
 
-    fn mount_path(
+    fn mount_path(&self, device: &devices::Device, _: &Storages) -> Result<path::PathBuf> {
-        &self,
-        device: &devices::Device,
-        _: &HashMap<String, Storage>,
-    ) -> Result<path::PathBuf> {
         Ok(self
             .local_infos
             .get(&device.name())
@@ -168,6 +167,10 @@ impl StorageExt for PhysicalDrivePartition {
         };
         Ok(())
     }
+
+    fn parent(&self, storages: &Storages) -> Result<Option<&Storage>> {
+        Ok(None)
+    }
 }
 
 impl fmt::Display for PhysicalDrivePartition {
@@ -189,7 +192,7 @@ impl fmt::Display for PhysicalDrivePartition {
 /// Interactively select physical storage from available disks in sysinfo.
 pub fn select_physical_storage(
     device: Device,
-    storages: &HashMap<String, Storage>,
+    storages: &Storages,
 ) -> Result<(String, PhysicalDrivePartition)> {
     trace!("select_physical_storage");
     // get disk info fron sysinfo
@@ -208,7 +211,7 @@ pub fn select_physical_storage(
     trace!("{}", disk_name);
     loop {
         disk_name = Text::new("Name for the disk:").prompt()?;
-        if storages.iter().all(|(k, v)| k != &disk_name) {
+        if storages.list.iter().all(|(k, v)| k != &disk_name) {
             break;
         }
         println!("The name {} is already used.", disk_name);