Merge pull request '20-dot-and-wildcard-paths-are-not-correctly-resolved-when-reffering-to-remote-files-or-ssh-targets' (#21) from 20-dot-and-wildcard-paths-are-not-correctly-resolved-when-reffering-to-remote-files-or-ssh-targets into master

Reviewed-on: https://stupstech.de/dev/Mr_Steppy/multi-ssh/pulls/21

src/command.rs

@@ -1,26 +1,34 @@
 use crate::log;
 use crate::logger::{LogLevel, Logger};
 use std::error::Error;
-use std::fmt::{Display, Formatter};
+use std::fmt::{Debug, Display, Formatter};
 use std::io;
 use std::iter::once;
 use std::process::{Command, ExitStatus, Output};
 
 pub trait LogRunnable {
-  fn run(&mut self, logger: &Logger) -> Result<(), SpecificExecutionError>;
+  fn run(&mut self, logger: &Logger) -> Result<(), CommandSpecificError<ExecutionError>>;
-  fn collect_output(&mut self) -> Result<Output, SpecificExecutionError>;
+  fn collect_output(&mut self) -> Result<Output, CommandSpecificError<ExecutionError>>;
+  fn collect_full_output(&mut self) -> Result<Output, CommandSpecificError<StartError>>;
 }
 
 impl LogRunnable for Command {
-  fn run(&mut self, logger: &Logger) -> Result<(), SpecificExecutionError> {
+  fn run(&mut self, logger: &Logger) -> Result<(), CommandSpecificError<ExecutionError>> {
-    run(self, logger).map_err(|error| SpecificExecutionError {
+    run(self, logger).map_err(|error| CommandSpecificError {
       command: self,
       error,
     })
   }
 
-  fn collect_output(&mut self) -> Result<Output, SpecificExecutionError> {
+  fn collect_output(&mut self) -> Result<Output, CommandSpecificError<ExecutionError>> {
-    collect_output(self, None).map_err(|error| SpecificExecutionError {
+    collect_output(self, None).map_err(|error| CommandSpecificError {
+      command: self,
+      error,
+    })
+  }
+
+  fn collect_full_output(&mut self) -> Result<Output, CommandSpecificError<StartError>> {
+    collect_full_output(self).map_err(|error| CommandSpecificError {
       command: self,
       error,
     })
@@ -46,7 +54,7 @@ fn collect_output(
   command: &mut Command,
   logger: Option<&Logger>,
 ) -> Result<Output, ExecutionError> {
-  let output = command.output()?; //pipes stdout and stderr automatically
+  let output = collect_full_output(command)?;
   if !output.status.success() {
     if let Some(logger) = logger {
       log!(logger, error, "{}", String::from_utf8_lossy(&output.stdout));
@@ -57,13 +65,20 @@ fn collect_output(
   Ok(output)
 }
 
-#[derive(Debug)]
+fn collect_full_output(command: &mut Command) -> Result<Output, StartError> {
-pub struct SpecificExecutionError<'a> {
+  Ok(command.output()?)
-  pub command: &'a Command,
-  pub error: ExecutionError,
 }
 
-impl Display for SpecificExecutionError<'_> {
+#[derive(Debug)]
+pub struct CommandSpecificError<'a, E> {
+  pub command: &'a Command,
+  pub error: E,
+}
+
+impl<E> Display for CommandSpecificError<'_, E>
+where
+  E: Display,
+{
   fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
     write!(
       f,
@@ -75,22 +90,52 @@ impl Display for SpecificExecutionError<'_> {
 }
 
 fn command_to_string(command: &Command) -> String {
-  once(command.get_program().to_string_lossy())
+  once(command.get_program().to_string_lossy().to_string())
-    .chain(command.get_args().map(|arg| arg.to_string_lossy()))
+    .chain(command.get_args().map(|arg| {
+      let arg_str = arg.to_string_lossy();
+      if arg_str.contains(' ') {
+        format!("\"{arg_str}\"")
+      } else {
+        arg_str.to_string()
+      }
+    }))
     .collect::<Vec<_>>()
     .join(" ")
 }
 
-impl Error for SpecificExecutionError<'_> {}
+impl<E> Error for CommandSpecificError<'_, E> where E: Debug + Display {}
+
+#[derive(Debug)]
+pub struct StartError(io::Error);
+
+impl From<io::Error> for StartError {
+  fn from(value: io::Error) -> Self {
+    StartError(value)
+  }
+}
+
+impl Display for StartError {
+  fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+    write!(f, "Failed to start command: {}", self.0)
+  }
+}
+
+impl Error for StartError {}
 
 #[derive(Debug)]
 pub enum ExecutionError {
-  StartError(io::Error),
+  StartError(StartError),
   BadExitStatus(ExitStatus),
 }
 
 impl From<io::Error> for ExecutionError {
   fn from(value: io::Error) -> Self {
+    Self::StartError(StartError(value))
+  }
+}
+
+impl From<StartError> for ExecutionError {
+  fn from(value: StartError) -> Self {
     Self::StartError(value)
   }
 }
@@ -104,10 +149,8 @@ impl From<ExitStatus> for ExecutionError {
 impl Display for ExecutionError {
   fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
     match self {
-      ExecutionError::StartError(e) => write!(f, "Failed to start command: {}", e),
+      ExecutionError::StartError(e) => Display::fmt(e, f),
-      ExecutionError::BadExitStatus(status) => {
+      ExecutionError::BadExitStatus(status) => write!(f, "Command failed with {}", status),
-        write!(f, "Command failed with {}", status)
-      }
     }
   }
 }
@@ -116,7 +159,7 @@ impl Error for ExecutionError {}
 
 #[cfg(test)]
 mod test {
-  use crate::command::{ExecutionError, LogRunnable, SpecificExecutionError};
+  use crate::command::{CommandSpecificError, ExecutionError, LogRunnable};
   use crate::logger::Logger;
   use std::path::PathBuf;
   use std::process::Command;
@@ -125,7 +168,7 @@ mod test {
   fn test_unknown_command() {
     let mut command = Command::new("python7");
     let Err(
-      e @ SpecificExecutionError {
+      e @ CommandSpecificError {
         error: ExecutionError::StartError(_),
         ..
       },
@@ -142,7 +185,7 @@ mod test {
   fn test_error() {
     let mut command = Command::new("python3");
     let Err(
-      e @ SpecificExecutionError {
+      e @ CommandSpecificError {
         error: ExecutionError::BadExitStatus(_),
         ..
       },

src/main.rs

@@ -6,18 +6,20 @@ mod os_string_builder;
 mod server;
 
 use crate::action::{Action, FileAction, ServerActions};
-use crate::command::{ExecutionError, LogRunnable, SpecificExecutionError};
+use crate::command::{CommandSpecificError, ExecutionError, LogRunnable};
 use crate::file::{FileMatcher, FileNameInfo};
 use crate::logger::{LogLevel, Logger};
 use crate::os_string_builder::ReplaceWithOsStr;
-use crate::server::ServerAddress;
+use crate::server::{RelativeLocalPathAnker, ServerAddress};
 use clap::{Parser, Subcommand, ValueEnum};
 use lazy_regex::{lazy_regex, Lazy, Regex};
 use server::{Server, ServerReference};
 use std::cell::LazyCell;
+use std::ffi::OsStr;
 use std::hash::Hash;
 use std::io::Write;
 use std::iter::once;
+use std::os::unix::ffi::OsStrExt;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::{env, fs, io};
@@ -178,14 +180,15 @@ fn main() -> Result<(), String> {
 
   match args.command {
     Command::Upload {
-      files,
+      mut files,
       file_server,
       old_version_policy,
-      upload_directory,
+      mut upload_directory,
       no_confirm,
       file_name,
     } => {
       require_non_empty_servers(&servers)?;
+      require_non_empty(&files, "files to upload")?;
       start_ssh_agent(&logger)?;
 
       //resolve file server
@@ -204,21 +207,44 @@ fn main() -> Result<(), String> {
       match &file_server {
         Some(file_server) => match &file_server.address {
           ServerAddress::Ssh { ssh_address } => {
-            for file in &files {
+            //canonicalize remote files -> also makes sure they exist
-              check_file_exists_on_server(file, ssh_address, &file_server.server_directory_path)?;
+            files = files
-            }
+              .iter()
-          }
+              .map(|file| {
-          ServerAddress::Localhost => {
+                let output = ShellCmd::new("ssh")
-            for file in &files {
+                  .arg(ssh_address)
-              check_local_file_exists(file_server.server_directory_path.join(file))?;
+                  .arg(osf!("realpath -e ") + file_server.server_directory_path.join(file))
-            }
+                  .collect_full_output()
+                  .map_err(|e| format!("Failed to canonicalize files: {e}"))?;
+
+                if !output.status.success() {
+                  Err(format!(
+                    "Path doesn't match any files on file-server: {}",
+                    file.to_string_lossy()
+                  ))?;
+                }
+
+                let denoted_files = output
+                  .stdout
+                  .split(|&b| b == b'\n') //split at line breaks
+                  .filter(|bytes| !bytes.is_empty()) //needed since realpath sometimes gives us empty lines
+                  .map(|bytes| PathBuf::from(OsStr::from_bytes(bytes)))
+                  .collect::<Vec<_>>();
+
+                Ok(denoted_files)
+              })
+              .collect::<Result<Vec<_>, String>>()?
+              .into_iter()
+              .flatten()
+              .collect();
+            log!(logger, debug, "canonical files: {files:?}");
           }
+          ServerAddress::Localhost => files
+            .iter()
+            .map(|file| file_server.server_directory_path.join(file))
+            .try_for_each(check_local_file_exists)?,
         },
-        None => {
+        None => files.iter().try_for_each(check_local_file_exists)?,
-          for file in &files {
-            check_local_file_exists(file)?;
-          }
-        }
       }
 
       let file_details = files
@@ -234,6 +260,15 @@ fn main() -> Result<(), String> {
       let actions = servers
         .iter()
         .map(|server| {
+          //on local server canonicalize upload_directory
+          if let ServerAddress::Localhost = &server.address {
+            //create upload directory if it doesn't exist
+            fs::create_dir_all(&upload_directory)
+              .map_err(|e| format!("Failed to create upload-directory: {e}"))?;
+
+            upload_directory = fs::canonicalize(&upload_directory)
+              .map_err(|e| format!("failed to resolve upload-directory: {e}"))?;
+          }
           let working_directory = server.server_directory_path.join(&upload_directory);
           Ok(ServerActions {
             server,
@@ -535,6 +570,7 @@ where
   Ok(())
 }
 
+#[allow(dead_code)]
 fn check_file_exists_on_server<P, S, D>(
   path: P,
   ssh_address: S,
@@ -552,11 +588,11 @@ where
     .collect_output()
   {
     Ok(_) => Ok(()), //file exists on file server
-    Err(SpecificExecutionError {
+    Err(CommandSpecificError {
       error: ExecutionError::BadExitStatus(_), //test failed
       ..
     }) => Err(format!(
-      "File '{}' doesn't exist on file server",
+      "File '{}' doesn't exist on file-server",
       full_path.to_string_lossy()
     )),
     Err(e) => Err(format!(
@@ -571,12 +607,17 @@ fn get_home_directory() -> Result<PathBuf, String> {
     .and_then(|home_dir| home_dir.ok_or("Failed to find home directory".to_string()))
 }
 
-fn require_non_empty_servers(servers: &[Server]) -> Result<(), String> {
+fn require_non_empty_servers<T>(servers: &[T]) -> Result<(), String> {
-  if servers.is_empty() {
+  require_non_empty(servers, "servers for this operation")
-    Err("You did not provide any servers for this operation. Please see --help".to_string())
+}
-  } else {
+
-    Ok(())
+fn require_non_empty<T>(slice: &[T], slice_name: &str) -> Result<(), String> {
+  if slice.is_empty() {
+    Err(format!(
+      "You did not provide any {slice_name}. Please see --help"
+    ))?
   }
+  Ok(())
 }
 
 fn start_ssh_agent(logger: &Logger) -> Result<(), String> {
@@ -614,7 +655,7 @@ fn parse_server_configuration(config_str: &str) -> Result<Vec<Server>, String> {
   config_str
     .split(',')
     .map(|server_entry| {
-      Server::from_str(server_entry)
+      Server::from_str(server_entry, RelativeLocalPathAnker::Home)
         .map_err(|e| format!("Invalid server entry '{server_entry}': {e}"))
     })
     .collect()
@@ -658,6 +699,7 @@ mod test {
     assert_eq!(PathBuf::from("/home"), joined);
   }
 
+  /// When renaming a file in a folder, the folder is relevant in the new name
   #[test]
   fn rename_experiment() {
     fs::rename("test-ressources/files/test", "test-ressources/files/test1")
@@ -665,4 +707,10 @@ mod test {
     fs::rename("test-ressources/files/test1", "test-ressources/files/test")
       .expect("failed to rename test1 file back to test");
   }
+
+  #[test]
+  fn mkdir_experiment() {
+    fs::create_dir_all("./test-ressources/files/../python")
+      .expect("failed to create directory with relative path");
+  }
 }

src/os_string_builder.rs

@@ -2,6 +2,7 @@ use std::ffi::{OsStr, OsString};
 use std::fmt::{Debug, Formatter};
 use std::hash::{Hash, Hasher};
 use std::ops::{Add, AddAssign};
+use std::path::Path;
 
 pub trait ReplaceWithOsStr<'a, Pattern = &'a str> {
   #[must_use]
@@ -56,6 +57,12 @@ impl AsRef<OsStr> for OsStringBuilder {
   }
 }
 
+impl AsRef<Path> for OsStringBuilder {
+  fn as_ref(&self) -> &Path {
+    self.result.as_ref()
+  }
+}
+
 impl<P> Add<P> for OsStringBuilder
 where
   P: AsRef<OsStr>,

src/server.rs

@@ -2,6 +2,7 @@ use crate::get_home_directory;
 use std::cell::LazyCell;
 use std::error::Error;
 use std::fmt::{Display, Formatter};
+use std::fs;
 use std::hash::{Hash, Hasher};
 use std::ops::Deref;
 use std::path::PathBuf;
@@ -71,7 +72,7 @@ impl FromStr for ServerReference {
   type Err = ServerReferenceParseError;
 
   fn from_str(s: &str) -> Result<Self, Self::Err> {
-    Server::from_str(s)
+    Server::from_str(s, RelativeLocalPathAnker::CurrentDirectory)
       .map(Self::Resolved)
       .or_else(|_| Ok(Self::Identifier(s.to_string())))
   }
@@ -124,22 +125,28 @@ impl Server {
       ServerAddress::Localhost => "this computer",
     }
   }
-}
 
-impl FromStr for Server {
+  pub fn from_str(
-  type Err = ServerParseError;
+    s: &str,
-
+    relative_local_path_anker: RelativeLocalPathAnker,
-  fn from_str(s: &str) -> Result<Self, Self::Err> {
+  ) -> Result<Self, ServerParseError> {
     s.split_once(':')
       .ok_or(ServerParseError::MissingServerDirectory)
       .and_then(|(identifier, server_directory)| {
         let address = ServerAddress::from_str(identifier);
-        let mut server_directory_path = PathBuf::from(server_directory);
+        let server_directory_path = match &address {
-        if let ServerAddress::Localhost = &address {
+          ServerAddress::Ssh { .. } => PathBuf::from(server_directory),
-          let home_directory = get_home_directory()
+          ServerAddress::Localhost => fs::canonicalize(match relative_local_path_anker {
-            .map_err(|e| ServerParseError::HomeDirectoryRequired { detail_message: e })?;
+            RelativeLocalPathAnker::Home => {
-          server_directory_path = home_directory.join(&server_directory_path);
+              let home_directory = get_home_directory()
-        }
+                .map_err(|e| ServerParseError::HomeDirectoryRequired { detail_message: e })?;
+              home_directory.join(server_directory)
+            }
+            RelativeLocalPathAnker::CurrentDirectory => PathBuf::from(server_directory),
+          })
+          .map_err(|_| ServerParseError::ServerDirectoryNonExistent)?,
+        };
+
         Ok(Self {
           address,
           server_directory_path,
@@ -162,6 +169,12 @@ impl Display for Server {
   }
 }
 
+#[derive(Debug, Clone, Eq, PartialEq, Hash)]
+pub enum RelativeLocalPathAnker {
+  Home,
+  CurrentDirectory,
+}
+
 #[derive(Debug, Clone, Eq, PartialEq, Hash)]
 pub enum ServerAddress {
   Ssh { ssh_address: String },
@@ -201,6 +214,7 @@ impl ServerAddress {
 #[derive(Debug)]
 pub enum ServerParseError {
   MissingServerDirectory,
+  ServerDirectoryNonExistent,
   HomeDirectoryRequired { detail_message: String },
 }
 
@@ -218,6 +232,9 @@ impl Display for ServerParseError {
         f,
         "localhost requires home directory, but: {detail_message}"
       ),
+      ServerParseError::ServerDirectoryNonExistent => {
+        write!(f, "The specified server directory doesn't exist")
+      }
     }
   }
 }