From 7e026ec229660ef3d1fcffb493a3317f5b701ac6 Mon Sep 17 00:00:00 2001 From: qwjyh Date: Sun, 10 Mar 2024 13:00:28 +0900 Subject: [PATCH] replace raw HashMap with Storages --- README.md | 1 + src/cmd_storage.rs | 68 +++++----- src/main.rs | 17 +-- src/storages.rs | 166 ++++++++++++++++++----- src/storages/directory.rs | 64 +++++---- src/storages/online_storage.rs | 11 +- src/storages/physical_drive_partition.rs | 21 +-- 7 files changed, 224 insertions(+), 124 deletions(-) diff --git a/README.md b/README.md index ef0e81c..db56373 100644 --- a/README.md +++ b/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 diff --git a/src/cmd_storage.rs b/src/cmd_storage.rs index ba6015c..8c14194 100644 --- a/src/cmd_storage.rs +++ b/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 = get_storages(&config_dir)?; - let mut storages: HashMap = 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 => { - if storages.is_empty() { + StorageType::S => { + 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 = 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, + 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) => { - s.kind() - }, - Storage::SubDirectory(s) => { - &s.notes - }, - Storage::Online(s) => { - &s.provider - }, + Storage::PhysicalStorage(s) => s.kind(), + Storage::SubDirectory(s) => &s.notes, + Storage::Online(s) => &s.provider, }; - writeln!( - writer, - " {}", - note - )?; + writeln!(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 = 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 { + 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) +} diff --git a/src/main.rs b/src/main.rs index e4d38d7..bdde0cb 100644 --- a/src/main.rs +++ b/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, target: String) -> Result { - 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, git2::Error> { if repo.is_empty()? { Ok(None) diff --git a/src/storages.rs b/src/storages.rs index 3e3c838..fda440d 100644 --- a/src/storages.rs +++ b/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, - SubDirectory, - Online, + /// Physical storage + P, + /// 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, + storages: &Storages, ) -> Result { 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> { + 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, + storages: &Storages, ) -> Result; /// 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>; } pub mod directory; @@ -147,36 +168,111 @@ pub mod local_info; pub mod online_storage; pub mod physical_drive_partition; -/// Get `HashMap` from devices.yml([devices::DEVICESFILE]). -/// If [devices::DEVICESFILE] isn't found, return empty vec. -pub fn get_storages(config_dir: &path::Path) -> Result> { - if let Some(storages_file) = fs::read_dir(&config_dir)? - .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))?; +#[derive(Debug, Serialize, Deserialize)] +pub struct Storages { + pub list: HashMap, +} + +impl Storages { + /// Construct empty [`Storages`] + pub fn new() -> Storages { + Storages { + list: HashMap::new(), + } + } + + /// Get [`Storage`] with `name`. + pub fn get(&self, name: &String) -> Option<&Storage> { + self.list.get(name) + } + + /// 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> { + // 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 { + 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 = - serde_yaml::from_reader(reader).context("Failed to read devices.yml")?; + let yaml: Storages = + 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`. -pub fn write_storages(config_dir: &path::Path, storages: HashMap) -> 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)) -} +// /// Get `HashMap` from devices.yml([devices::DEVICESFILE]). +// /// If [devices::DEVICESFILE] isn't found, return empty vec. +// pub fn get_storages(config_dir: &path::Path) -> Result> { +// if let Some(storages_file) = fs::read_dir(&config_dir)? +// .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 = +// 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) -> 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)) +// } diff --git a/src/storages/directory.rs b/src/storages/directory.rs index 2ac849f..b0aca20 100644 --- a/src/storages/directory.rs +++ b/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, + storages: &Storages, ) -> Result { 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) -> 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( - &self, - device: &devices::Device, - storages: &HashMap, - ) -> Result { - let parent = self.parent(&storages)?; - let parent_mount_path = parent.mount_path(&device, &storages)?; + fn mount_path(&self, device: &devices::Device, storages: &Storages) -> Result { + let parent_mount_path = self + .parent(&storages)? + .context("Can't find parent storage")? + .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, + storages: &Storages, ) -> Result { 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> { + 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 = HashMap::new(); - storages.insert( - physical.name().to_string(), - Storage::PhysicalStorage(physical), - ); - storages.insert( - directory.name().to_string(), - Storage::SubDirectory(directory), - ); + let mut storages = Storages::new(); + storages.add(storages::Storage::PhysicalStorage(physical)).unwrap(); + storages.add(Storage::SubDirectory(directory)).unwrap(); // 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(), diff --git a/src/storages/online_storage.rs b/src/storages/online_storage.rs index d07d74b..b6b6203 100644 --- a/src/storages/online_storage.rs +++ b/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, + _storages: &Storages, ) -> Result { Ok(self .local_infos @@ -81,6 +81,13 @@ impl StorageExt for OnlineStorage { }; Ok(()) } + + fn parent( + &self, + storages: &Storages, + ) -> Result> { + Ok(None) + } } impl fmt::Display for OnlineStorage { diff --git a/src/storages/physical_drive_partition.rs b/src/storages/physical_drive_partition.rs index 14afe66..b86cea2 100644 --- a/src/storages/physical_drive_partition.rs +++ b/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( - &self, - device: &devices::Device, - _: &HashMap, - ) -> Result { + fn mount_path(&self, device: &devices::Device, _: &Storages) -> Result { Ok(self .local_infos .get(&device.name()) @@ -168,6 +167,10 @@ impl StorageExt for PhysicalDrivePartition { }; Ok(()) } + + fn parent(&self, storages: &Storages) -> Result> { + 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, + 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);