redesign storage add Commands

- replace storage add args with subcommands of physical, directory,
  online
- to make argument dependencies clearer

README.md

@@ -1,14 +1,19 @@
 # TODO:
 - [x] split subcommands to functions
 - [x] write test for init subcommand
-  - [ ] write test with existing repo
+  - [x] write test with existing repo
   - [x] with ssh credential
     - [x] ssh-agent
     - [x] specify key
+- [ ] write test for storage subcommand
+  - [ ] storage add online
+  - [ ] storage add directory
+  - [ ] storage list
 - [ ] add storage remove command
 - [ ] add sync subcommand
 - [ ] add check subcommand
-- [ ] reorganize cmd option for storage
+- [x] reorganize cmd option for storage
-  - [ ] use subcommand
+  - [x] use subcommand
+- [ ] no commit option
 
 <!-- vim: set sw=2 ts=2:  -->

src/cmd_args.rs

@@ -3,6 +3,7 @@
 use crate::StorageType;
 use crate::path;
 use crate::PathBuf;
+use clap::Args;
 use clap::{Parser, Subcommand};
 use clap_verbosity_flag::Verbosity;
 
@@ -50,7 +51,7 @@ pub(crate) enum Commands {
     Check {},
 }
 
-#[derive(clap::Args, Debug)]
+#[derive(Args, Debug)]
 #[command(args_conflicts_with_subcommands = true)]
 pub(crate) struct StorageArgs {
     #[command(subcommand)]
@@ -60,14 +61,7 @@ pub(crate) struct StorageArgs {
 #[derive(Subcommand, Debug)]
 pub(crate) enum StorageCommands {
     /// Add new storage.
-    Add {
+    Add(StorageAddArgs),
-        #[arg(value_enum)]
-        storage_type: StorageType,
-
-        // TODO: set this require and select matching disk for physical
-        #[arg(short, long, value_name = "PATH")]
-        path: Option<PathBuf>,
-    },
     /// List all storages.
     List {
         /// Show note on the storages.
@@ -87,3 +81,50 @@ pub(crate) enum StorageCommands {
         path: path::PathBuf,
     },
 }
+
+#[derive(Args, Debug)]
+pub(crate) struct StorageAddArgs {
+    #[command(subcommand)]
+    pub(crate) command: StorageAddCommands,
+}
+
+#[derive(Subcommand, Debug)]
+pub(crate) enum StorageAddCommands {
+    /// Physical drive partition.
+    Physical {
+        /// Unique name for the storage.
+        name: String,
+        /// Path where the storage is mounted on this device.
+        /// leave blank to fetch system info automatically.
+        path: Option<PathBuf>,
+    },
+    /// Sub directory of other storages.
+    Directory {
+        /// Unique name for the storage.
+        name: String,
+        /// Path where the storage is mounted on this device.
+        path: PathBuf,
+        /// Additional info. Empty by default.
+        #[arg(short, long, default_value = "")]
+        notes: String,
+        /// Device specific alias for the storage.
+        #[arg(short, long)]
+        alias: String,
+    },
+    /// Online storage.
+    Online {
+        /// Unique name for the storage.
+        name: String,
+        /// Path where the storage is mounted on this device.
+        path: PathBuf,
+        /// Provider name (for the common information).
+        #[arg(short, long)]
+        provider: String,
+        /// Capacity in bytes.
+        #[arg(short, long)]
+        capacity: u64,
+        /// Device specific alias for the storage.
+        #[arg(short, long)]
+        alias: String,
+    }
+}

src/cmd_init.rs

@@ -76,6 +76,10 @@ pub(crate) fn cmd_init(
     ssh_key: Option<PathBuf>,
     config_dir: &path::PathBuf,
 ) -> Result<()> {
+    if config_dir.join(DEVICESFILE).exists() {
+        debug!("{} already exists.", DEVICESFILE);
+        return Err(anyhow!("This device is already added."));
+    }
     // validate device name
     if device_name.chars().count() == 0 {
         log::error!("Device name cannnot by empty");

src/cmd_storage.rs

@@ -16,60 +16,124 @@ use unicode_width::{self, UnicodeWidthStr};
 
 use crate::{
     add_and_commit,
-    cmd_args::Cli,
+    cmd_args::{Cli, StorageAddCommands},
     devices::{self, Device},
     inquire_filepath_completer::FilePathCompleter,
     storages::{
-        self, directory, local_info, physical_drive_partition, Storage, StorageExt, StorageType,
+        self, directory, local_info,
-        Storages,
+        physical_drive_partition::{self, PhysicalDrivePartition},
+        Storage, StorageExt, StorageType, Storages,
     },
 };
 
 pub(crate) fn cmd_storage_add(
-    storage_type: storages::StorageType,
+    args: StorageAddCommands,
-    path: Option<PathBuf>,
     repo: Repository,
     config_dir: &PathBuf,
 ) -> Result<()> {
-    trace!("Storage Add {:?}, {:?}", storage_type, path);
+    trace!("Storage Add with args: {:?}", args);
     // Get storages
     let mut storages = Storages::read(&config_dir)?;
     trace!("found storages: {:?}", storages);
 
     let device = devices::get_device(&config_dir)?;
-    let storage = match storage_type {
+    let storage = match args {
-        StorageType::P => {
+        StorageAddCommands::Physical { name, path } => {
-            let use_sysinfo = {
+            if !is_unique_name(&name, &storages) {
-                let options = vec![
+                return Err(anyhow!(
-                    "Fetch disk information automatically.",
+                    "The name {} is already used for another storage.",
-                    "Type disk information manually.",
+                    name
-                ];
+                ));
-                let ans = Select::new(
+            }
-                    "Do you fetch disk information automatically? (it may take a few minutes)",
+            let use_sysinfo = path.is_none();
-                    options,
+            let storage = if use_sysinfo {
-                )
+                physical_drive_partition::select_physical_storage(name, device)?
-                .prompt()
+            } else {
-                .context("Failed to get response. Please try again.")?;
+                manually_construct_physical_drive_partition(name, path.unwrap(), &device)?
-                match ans {
+            };
-                    "Fetch disk information automatically." => true,
+            println!("storage: {}: {:?}", storage.name(), storage);
-                    _ => false,
+            Storage::PhysicalStorage(storage)
+        }
+        StorageAddCommands::Directory {
+            name,
+            path,
+            notes,
+            alias,
+        } => {
+            if !is_unique_name(&name, &storages) {
+                return Err(anyhow!(
+                    "The name {} is already used for another storage.",
+                    name
+                ));
+            }
+            if storages.list.is_empty() {
+                return Err(anyhow!(
+                    "No storages found. Please add at least 1 physical/online storage first to add sub directory."
+                ));
+            }
+            trace!("SubDirectory arguments: path: {:?}", path);
+            // Nightly feature std::path::absolute
+            let path = path.canonicalize()?;
+            trace!("canonicalized: path: {:?}", path);
+
+            let storage = directory::Directory::try_from_device_path(
+                name, path, notes, alias, &device, &storages,
+            )?;
+            Storage::SubDirectory(storage)
+        }
+        StorageAddCommands::Online {
+            name,
+            path,
+            provider,
+            capacity,
+            alias,
+        } => {
+            if !is_unique_name(&name, &storages) {
+                return Err(anyhow!(
+                    "The name {} is already used for another storage.",
+                    name
+                ));
+            }
+            let storage = storages::online_storage::OnlineStorage::new(
+                name, provider, capacity, alias, path, &device,
+            );
+            Storage::Online(storage)
         }
     };
-            let storage = if use_sysinfo {
+
-                // select storage
+    // add to storages
-                physical_drive_partition::select_physical_storage(device, &storages)?
+    let new_storage_name = storage.name().clone();
-            } else {
+    let new_storage_type = storage.typename().to_string();
-                let mut name = String::new();
+    storages.add(storage)?;
-                loop {
+    trace!("updated storages: {:?}", storages);
-                    name = Text::new("Name for the storage:")
+
-                        .with_validator(min_length!(0, "At least 1 character"))
+    // write to file
-                        .prompt()
+    storages.write(&config_dir)?;
-                        .context("Failed to get Name")?;
+
-                    if storages.list.iter().all(|(k, _v)| k != &name) {
+    // commit
-                        break;
+    add_and_commit(
-                    }
+        &repo,
-                    println!("The name {} is already used.", name);
+        &Path::new(storages::STORAGESFILE),
-                }
+        &format!(
+            "Add new storage({}): {}",
+            new_storage_type, new_storage_name
+        ),
+    )?;
+
+    println!("Added new storage.");
+    trace!("Finished adding storage");
+    Ok(())
+}
+
+fn is_unique_name(newname: &String, storages: &Storages) -> bool {
+    storages.list.iter().all(|(name, _)| name != newname)
+}
+
+fn manually_construct_physical_drive_partition(
+    name: String,
+    path: PathBuf,
+    device: &Device,
+) -> Result<PhysicalDrivePartition> {
     let kind = Text::new("Kind of storage (ex. SSD):")
         .prompt()
         .context("Failed to get kind.")?;
@@ -83,13 +147,11 @@ pub(crate) fn cmd_storage_add(
     let is_removable = Confirm::new("Is removable")
         .prompt()
         .context("Failed to get is_removable")?;
-                let mount_path: PathBuf = PathBuf::from(
+    let alias = Text::new("Alias of the storage for this device")
-                    Text::new("mount path:")
+        .prompt()
-                        .with_autocomplete(FilePathCompleter::default())
+        .context("Failed to get alias.")?;
-                        .prompt()?,
+    let local_info = local_info::LocalInfo::new(alias, path);
-                );
+    Ok(physical_drive_partition::PhysicalDrivePartition::new(
-                let local_info = local_info::LocalInfo::new("".to_string(), mount_path);
-                physical_drive_partition::PhysicalDrivePartition::new(
         name,
         kind,
         capacity,
@@ -97,99 +159,17 @@ pub(crate) fn cmd_storage_add(
         is_removable,
         local_info,
         &device,
-                )
+    ))
-            };
-            println!("storage: {}: {:?}", storage.name(), storage);
-            Storage::PhysicalStorage(storage)
-        }
-        StorageType::S => {
-            if storages.list.is_empty() {
-                return Err(anyhow!(
-                    "No storages found. Please add at least 1 physical/online storage first to add sub directory."
-                ));
-            }
-            let path = path.unwrap_or_else(|| {
-                let mut cmd = Cli::command();
-                // TODO: weired def of cmd argument
-                cmd.error(
-                    ErrorKind::MissingRequiredArgument,
-                    "<PATH> is required with sub-directory",
-                )
-                .exit();
-            });
-            trace!("SubDirectory arguments: path: {:?}", path);
-            // Nightly feature std::path::absolute
-            let path = path.canonicalize()?;
-            trace!("canonicalized: path: {:?}", path);
-
-            let key_name = ask_unique_name(&storages, "sub-directory".to_string())?;
-            let notes = Text::new("Notes for this sub-directory:").prompt()?;
-            let storage = directory::Directory::try_from_device_path(
-                key_name, path, notes, &device, &storages,
-            )?;
-            Storage::SubDirectory(storage)
-        }
-        StorageType::O => {
-            let path = path.unwrap_or_else(|| {
-                let mut cmd = Cli::command();
-                cmd.error(
-                    ErrorKind::MissingRequiredArgument,
-                    "<PATH> is required with sub-directory",
-                )
-                .exit();
-            });
-            let mut name = String::new();
-            loop {
-                name = Text::new("Name for the storage:")
-                    .with_validator(min_length!(0, "At least 1 character"))
-                    .prompt()
-                    .context("Failed to get Name")?;
-                if storages.list.iter().all(|(k, _v)| k != &name) {
-                    break;
-                }
-                println!("The name {} is already used.", name);
-            }
-            let provider = Text::new("Provider:")
-                .prompt()
-                .context("Failed to get provider")?;
-            let capacity: u64 = CustomType::<u64>::new("Capacity (byte):")
-                .with_error_message("Please type number.")
-                .prompt()
-                .context("Failed to get capacity.")?;
-            let alias = Text::new("Alias:")
-                .prompt()
-                .context("Failed to get provider")?;
-            let storage = storages::online_storage::OnlineStorage::new(
-                name, provider, capacity, alias, path, &device,
-            );
-            Storage::Online(storage)
-        }
-    };
-
-    // add to storages
-    let new_storage_name = storage.name().clone();
-    storages.add(storage)?;
-    trace!("updated storages: {:?}", storages);
-
-    // write to file
-    storages.write(&config_dir)?;
-
-    // commit
-    add_and_commit(
-        &repo,
-        &Path::new(storages::STORAGESFILE),
-        &format!("Add new storage(physical drive): {}", new_storage_name),
-    )?;
-
-    println!("Added new storage.");
-    trace!("Finished adding storage");
-    Ok(())
 }
 
 pub(crate) fn cmd_storage_list(config_dir: &PathBuf, with_note: bool) -> Result<()> {
     // Get storages
     let storages = Storages::read(&config_dir)?;
     trace!("found storages: {:?}", storages);
+    if storages.list.is_empty() {
+        println!("No storages found");
+        return Ok(());
+    }
     let device = devices::get_device(&config_dir)?;
     let mut stdout = io::BufWriter::new(io::stdout());
     write_storages_list(&mut stdout, &storages, &device, with_note)?;

src/main.rs

@@ -71,9 +71,9 @@ fn main() -> Result<()> {
             )?;
             trace!("repo state: {:?}", repo.state());
             match storage.command {
-                StorageCommands::Add { storage_type, path } => {
+                StorageCommands::Add(storageargs) => {
-                    cmd_storage::cmd_storage_add(storage_type, path, repo, &config_dir)?
+                    cmd_storage::cmd_storage_add(storageargs.command, repo, &config_dir)?
-                }
+                },
                 StorageCommands::List { long } => cmd_storage::cmd_storage_list(&config_dir, long)?,
                 StorageCommands::Bind {
                     storage: storage_name,

src/storages/directory.rs

@@ -15,10 +15,14 @@ use super::{local_info::LocalInfo, Storage, StorageExt, Storages};
 /// Subdirectory of other [Storage]s.
 #[derive(Serialize, Deserialize, Debug)]
 pub struct Directory {
+    /// ID.
     name: String,
+    /// ID of parent storage.
     parent: String,
+    /// Relative path to the parent storage.
     relative_path: path::PathBuf,
     pub notes: String,
+    /// Device and localinfo pairs.
     local_infos: HashMap<String, LocalInfo>,
 }
 
@@ -47,6 +51,7 @@ impl Directory {
         name: String,
         path: path::PathBuf,
         notes: String,
+        alias: String,
         device: &devices::Device,
         storages: &Storages,
     ) -> Result<Directory> {
@@ -69,7 +74,7 @@ impl Directory {
             })
             .context(format!("Failed to compare diff of paths"))?;
         trace!("Selected parent: {}", parent_name);
-        let local_info = LocalInfo::new("".to_string(), path);
+        let local_info = LocalInfo::new(alias, path);
         Ok(Directory::new(
             name,
             parent_name.clone(),

src/storages/online_storage.rs

@@ -16,13 +16,19 @@ use super::{
 
 #[derive(Serialize, Deserialize, Debug)]
 pub struct OnlineStorage {
+    /// ID.
     name: String,
+    /// Provider string (for the common information).
     pub provider: String,
+    /// Capacity in bytes.
     capacity: u64,
+    /// Device and local info pairs.
     local_infos: HashMap<String, LocalInfo>,
 }
 
 impl OnlineStorage {
+    /// # Arguments
+    /// - alias: for [`LocalInfo`]
     pub fn new(
         name: String,
         provider: String,

src/storages/physical_drive_partition.rs

@@ -7,11 +7,8 @@ use anyhow::{anyhow, Context, Result};
 use byte_unit::Byte;
 use inquire::Text;
 use serde::{Deserialize, Serialize};
-use std::path::{self, PathBuf};
+use std::path;
-use std::{
+use std::{collections::HashMap, fmt};
-    collections::{hash_map::RandomState, HashMap},
-    fmt,
-};
 use sysinfo::{Disk, DiskExt, SystemExt};
 
 use super::local_info::{self, LocalInfo};
@@ -191,8 +188,8 @@ impl fmt::Display for PhysicalDrivePartition {
 
 /// Interactively select physical storage from available disks in sysinfo.
 pub fn select_physical_storage(
+    disk_name: String,
     device: Device,
-    storages: &Storages,
 ) -> Result<PhysicalDrivePartition> {
     trace!("select_physical_storage");
     // get disk info fron sysinfo
@@ -206,22 +203,11 @@ pub fn select_physical_storage(
         trace!("{:?}", disk)
     }
     let disk = select_sysinfo_disk(&sys_disks)?;
-    // name the disk
-    let mut disk_name = String::new();
-    trace!("{}", disk_name);
-    loop {
-        disk_name = Text::new("Name for the disk:").prompt()?;
-        if storages.list.iter().all(|(k, v)| k != &disk_name) {
-            break;
-        }
-        println!("The name {} is already used.", disk_name);
-    }
-    trace!("selected name: {}", disk_name);
     let storage = PhysicalDrivePartition::try_from_sysinfo_disk(&disk, disk_name, device)?;
     Ok(storage)
 }
 
-pub fn select_sysinfo_disk(sysinfo: &sysinfo::System) -> Result<&Disk> {
+fn select_sysinfo_disk(sysinfo: &sysinfo::System) -> Result<&Disk> {
     let available_disks = sysinfo
         .disks()
         .iter()