cp: fix "delete before copy" logic. Add tests.

src/uu/cp/src/cp.rs

@@ -2,7 +2,7 @@
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
-// spell-checker:ignore (ToDO) copydir ficlone fiemap ftruncate linkgs lstat nlink nlinks pathbuf pwrite reflink strs xattrs symlinked deduplicated advcpmv nushell IRWXG IRWXO IRWXU IRWXUGO IRWXU IRWXG IRWXO IRWXUGO
+// spell-checker:ignore (ToDO) copydir ficlone fiemap ftruncate linkgs lstat nlink nlinks pathbuf pwrite reflink strs xattrs symlinked deduplicated advcpmv nushell IRWXG IRWXO IRWXU IRWXUGO IRWXU IRWXG IRWXO IRWXUGO unwritable
 
 use quick_error::quick_error;
 use std::cmp::Ordering;
@@ -27,13 +27,13 @@
 
 use platform::copy_on_write;
 use uucore::display::Quotable;
-use uucore::error::{set_exit_code, UClapError, UError, UResult, UUsageError};
+use uucore::error::{set_exit_code, UClapError, UError, UResult, USimpleError, UUsageError};
 use uucore::fs::{
     are_hardlinks_to_same_file, canonicalize, get_filename, is_symlink_loop,
     path_ends_with_terminator, paths_refer_to_same_file, FileInformation, MissingHandling,
     ResolveMode,
 };
-use uucore::{backup_control, update_control};
+use uucore::{backup_control, show, update_control};
 // These are exposed for projects (e.g. nushell) that want to create an `Options` value, which
 // requires these enum.
 pub use uucore::{backup_control::BackupMode, update_control::UpdateMode};
@@ -1428,7 +1428,7 @@
                     let mode: mode_t = me.mode() as mode_t;
 
                     // It looks like this extra information is added to the prompt iff the file's user write bit is 0
-                    //  write permission, owner
+                    // write permission, owner
                     if uucore::has!(mode, S_IWUSR) {
                         None
                     } else {
@@ -1454,22 +1454,36 @@
 }
 
 impl OverwriteMode {
-    fn verify(&self, path: &Path, debug: bool) -> CopyResult<()> {
+    fn verify(&self, path: &Path) -> CopyResult<()> {
         match *self {
             Self::NoClobber => {
-                if debug {
-                    println!("skipped {}", path.quote());
-                }
+                // TODO
+                // Revert to 1_i32 when "mv/update" is fixed
+                show!(USimpleError::new(
+                    0_i32,
+                    format!("not replacing {}", path.quote())
+                ));
+
                 Err(Error::Skipped(false))
             }
-            Self::Interactive(_) => {
+            Self::Interactive(cl) => {
                 let prompt_yes_result = if let Some((octal, human_readable)) =
                     file_mode_for_interactive_overwrite(path)
                 {
-                    prompt_yes!(
-                        "replace {}, overriding mode {octal} ({human_readable})?",
-                        path.quote()
-                    )
+                    match cl {
+                        ClobberMode::Force => {
+                            prompt_yes!(
+                                "replace {}, overriding mode {octal} ({human_readable})?",
+                                path.quote()
+                            )
+                        }
+                        _ => {
+                            prompt_yes!(
+                                "unwritable {} (mode {octal}, {human_readable}); try anyway?",
+                                path.quote()
+                            )
+                        }
+                    }
                 } else {
                     prompt_yes!("overwrite {}?", path.quote())
                 };
@@ -1710,21 +1724,22 @@
 }
 
 /// Back up, remove, or leave intact the destination file, depending on the options.
+/// Returns true if `dest` was deleted.
 fn handle_existing_dest(
     source: &Path,
     dest: &Path,
     options: &Options,
     source_in_command_line: bool,
     copied_files: &HashMap<FileInformation, PathBuf>,
-) -> CopyResult<()> {
+) -> Result<bool, Error> {
     // Disallow copying a file to itself, unless `--force` and
     // `--backup` are both specified.
     if is_forbidden_to_copy_to_same_file(source, dest, options, source_in_command_line) {
         return Err(format!("{} and {} are the same file", source.quote(), dest.quote()).into());
     }
 
     if options.update != UpdateMode::ReplaceIfOlder {
-        options.overwrite.verify(dest, options.debug)?;
+        options.overwrite.verify(dest)?;
     }
 
     let mut is_dest_removed = false;
@@ -1742,41 +1757,89 @@
             backup_dest(dest, &backup_path, is_dest_removed)?;
         }
     }
-    if !is_dest_removed {
+
+    let dest_was_removed = if is_dest_removed {
+        // TODO
+        // Should this be true?
+        false
+    } else {
         delete_dest_if_needed_and_allowed(
             source,
             dest,
             options,
             source_in_command_line,
             copied_files,
-        )?;
-    }
+        )?
+    };
 
-    Ok(())
+    Ok(dest_was_removed)
+}
+
+/// Returns true if `path` is writeable by the current group/user, otherwise returns false
+#[cfg(unix)]
+fn is_file_writeable(path: &Path) -> bool {
+    // 0 indicates that that the call to `access` succeeded, which in this case would mean that `path` is writeable
+    const SUCCESS: libc::c_int = 0;
+
+    // It looks like the pattern in the codebase is to unwrap the return value of `CString::new`
+    let path_cstring = CString::new(path.as_os_str().as_bytes().to_vec()).unwrap();
+
+    let path_pointer: *const libc::c_char = path_cstring.as_ptr().cast::<libc::c_char>();
+
+    let access_result = unsafe {
+        // Check if the current group/user can can write to `path`
+        libc::access(path_pointer, libc::W_OK)
+    };
+
+    // Ensure `path_pointer` remains valid through FFI call
+    drop(path_cstring);
+
+    access_result == SUCCESS
 }
 
 /// Checks if:
 /// * `dest` needs to be deleted before the copy operation can proceed
 /// * the provided options allow this deletion
 ///
 /// If so, deletes `dest`.
+/// Returns a boolean indicating if `dest` was deleted.
+/// This boolean is needed later for verbose mode.
 fn delete_dest_if_needed_and_allowed(
     source: &Path,
     dest: &Path,
     options: &Options,
     source_in_command_line: bool,
     copied_files: &HashMap<FileInformation, PathBuf>,
-) -> CopyResult<()> {
+) -> Result<bool, Error> {
     let delete_dest = match options.overwrite {
         OverwriteMode::Clobber(cl) | OverwriteMode::Interactive(cl) => {
             match cl {
-                // FIXME: print that the file was removed if --verbose is enabled
                 ClobberMode::Force => {
-                    // TODO
-                    // Using `readonly` here to check if `dest` needs to be deleted is not correct:
-                    // "On Unix-based platforms this checks if any of the owner, group or others write permission bits are set. It does not check if the current user is in the file's assigned group. It also does not check ACLs. Therefore the return value of this function cannot be relied upon to predict whether attempts to read or write the file will actually succeed."
-                    // This results in some copy operations failing, because this necessary deletion is being skipped.
-                    is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly()
+                    // Delete `dest` if there is a symlink loop
+                    if is_symlink_loop(dest) {
+                        true
+                    } else {
+                        // Retain block to ensure only one branch is included
+                        let dest_is_writeable = {
+                            #[cfg(unix)]
+                            {
+                                is_file_writeable(dest)
+                            }
+
+                            #[cfg(not(unix))]
+                            {
+                                // TODO
+                                // `readonly` is probably not correct to use here.
+                                // "Not read-only" (as defined by the Rust Standard Library) does not necessarily mean "writeable by the current group/user".
+                                // Relying on `readonly` may cause necessary deletions to be skipped.
+                                // This would result in the subsequent copy operation failing.
+                                !fs::metadata(dest)?.permissions().readonly()
+                            }
+                        };
+
+                        // Delete `dest` if it is not writeable
+                        !dest_is_writeable
+                    }
                 }
                 ClobberMode::RemoveDestination => true,
                 ClobberMode::Standard => {
@@ -1812,7 +1875,7 @@
         fs::remove_file(dest)?;
     }
 
-    Ok(())
+    Ok(delete_dest)
 }
 
 /// Decide whether the given path exists.
@@ -1870,18 +1933,19 @@
     progress_bar: &Option<ProgressBar>,
     source: &Path,
     dest: &Path,
+    dest_was_deleted: bool,
 ) {
     if let Some(pb) = progress_bar {
         // Suspend (hide) the progress bar so the println won't overlap with the progress bar.
         pb.suspend(|| {
-            print_paths(parents, source, dest);
+            print_paths(parents, source, dest, dest_was_deleted);
         });
     } else {
-        print_paths(parents, source, dest);
+        print_paths(parents, source, dest, dest_was_deleted);
     }
 }
 
-fn print_paths(parents: bool, source: &Path, dest: &Path) {
+fn print_paths(parents: bool, source: &Path, dest: &Path, dest_was_deleted: bool) {
     if parents {
         // For example, if copying file `a/b/c` and its parents
         // to directory `d/`, then print
@@ -1895,6 +1959,10 @@
     }
 
     println!("{}", context_for(source, dest));
+
+    if dest_was_deleted {
+        println!("removed {}", dest.quote());
+    }
 }
 
 /// Handles the copy mode for a file copy operation.
@@ -2000,7 +2068,7 @@
                         if src_time <= dest_time {
                             return Ok(());
                         } else {
-                            options.overwrite.verify(dest, options.debug)?;
+                            options.overwrite.verify(dest)?;
 
                             copy_helper(
                                 source,
@@ -2161,6 +2229,8 @@
         fs::remove_file(dest)?;
     }
 
+    let mut dest_was_deleted = false;
+
     if file_or_link_exists(dest)
         && (!options.attributes_only
             || matches!(
@@ -2186,7 +2256,10 @@
                 }
             }
         }
-        handle_existing_dest(source, dest, options, source_in_command_line, copied_files)?;
+
+        dest_was_deleted =
+            handle_existing_dest(source, dest, options, source_in_command_line, copied_files)?;
+
         if are_hardlinks_to_same_file(source, dest) {
             if options.copy_mode == CopyMode::Copy && options.backup != BackupMode::NoBackup {
                 return Ok(());
@@ -2212,7 +2285,13 @@
     }
 
     if options.verbose {
-        print_verbose_output(options.parents, progress_bar, source, dest);
+        print_verbose_output(
+            options.parents,
+            progress_bar,
+            source,
+            dest,
+            dest_was_deleted,
+        );
     }
 
     if options.preserve_hard_links() {
@@ -2362,7 +2441,7 @@
         File::create(dest).context(dest.display().to_string())?;
     } else if source_is_fifo && options.recursive && !options.copy_contents {
         #[cfg(unix)]
-        copy_fifo(dest, options.overwrite, options.debug)?;
+        copy_fifo(dest, options.overwrite)?;
     } else if source_is_symlink {
         copy_link(source, dest, symlinked_files)?;
     } else {
@@ -2387,9 +2466,9 @@
 // "Copies" a FIFO by creating a new one. This workaround is because Rust's
 // built-in fs::copy does not handle FIFOs (see rust-lang/rust/issues/79390).
 #[cfg(unix)]
-fn copy_fifo(dest: &Path, overwrite: OverwriteMode, debug: bool) -> CopyResult<()> {
+fn copy_fifo(dest: &Path, overwrite: OverwriteMode) -> CopyResult<()> {
     if dest.exists() {
-        overwrite.verify(dest, debug)?;
+        overwrite.verify(dest)?;
         fs::remove_file(dest)?;
     }