chown: show message if file doesn't exist

* print the message "failed to change ownership of" when we try to change a non existing file. * replace the 4-tuple returned by parse_gid_uid_and_filter by GidUidOwnerFilter struct. * chain the test in one expression. * chown: remove unused var "result" in test --------- Co-authored-by: Daniel Hofstetter <[email protected]>
uutils · May 21, 2023 · 0130a07 · 0130a07
1 parent 70765ee
commit 0130a07
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 20 deletions.
diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs
@@ -10,7 +10,7 @@
 use uucore::display::Quotable;
 pub use uucore::entries;
 use uucore::error::{FromIo, UResult, USimpleError};
-use uucore::perms::{chown_base, options, IfFrom};
+use uucore::perms::{chown_base, options, GidUidOwnerFilter, IfFrom};
 use uucore::{format_usage, help_about, help_usage};
 
 use clap::{crate_version, Arg, ArgAction, ArgMatches, Command};
@@ -21,16 +21,22 @@ use std::os::unix::fs::MetadataExt;
 const ABOUT: &str = help_about!("chgrp.md");
 const USAGE: &str = help_usage!("chgrp.md");
 
-fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<(Option<u32>, Option<u32>, IfFrom)> {
+fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<GidUidOwnerFilter> {
+    let mut raw_group: String = String::new();
     let dest_gid = if let Some(file) = matches.get_one::<String>(options::REFERENCE) {
         fs::metadata(file)
-            .map(|meta| Some(meta.gid()))
+            .map(|meta| {
+                let gid = meta.gid();
+                raw_group = entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string());
+                Some(gid)
+            })
             .map_err_context(|| format!("failed to get attributes of {}", file.quote()))?
     } else {
         let group = matches
             .get_one::<String>(options::ARG_GROUP)
             .map(|s| s.as_str())
             .unwrap_or_default();
+        raw_group = group.to_string();
         if group.is_empty() {
             None
         } else {
@@ -45,7 +51,12 @@ fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<(Option<u32>, Option<u32>,
             }
         }
     };
-    Ok((dest_gid, None, IfFrom::All))
+    Ok(GidUidOwnerFilter {
+        dest_gid,
+        dest_uid: None,
+        raw_owner: raw_group,
+        filter: IfFrom::All,
+    })
 }
 
 #[uucore::main]

diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs
@@ -9,7 +9,7 @@
 
 use uucore::display::Quotable;
 pub use uucore::entries::{self, Group, Locate, Passwd};
-use uucore::perms::{chown_base, options, IfFrom};
+use uucore::perms::{chown_base, options, GidUidOwnerFilter, IfFrom};
 use uucore::{format_usage, help_about, help_usage};
 
 use uucore::error::{FromIo, UResult, USimpleError};
@@ -23,7 +23,7 @@ static ABOUT: &str = help_about!("chown.md");
 
 const USAGE: &str = help_usage!("chown.md");
 
-fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult<(Option<u32>, Option<u32>, IfFrom)> {
+fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult<GidUidOwnerFilter> {
     let filter = if let Some(spec) = matches.get_one::<String>(options::FROM) {
         match parse_spec(spec, ':')? {
             (Some(uid), None) => IfFrom::User(uid),
@@ -37,17 +37,34 @@ fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult<(Option<u32>, Optio
 
     let dest_uid: Option<u32>;
     let dest_gid: Option<u32>;
+    let raw_owner: String;
     if let Some(file) = matches.get_one::<String>(options::REFERENCE) {
         let meta = fs::metadata(file)
             .map_err_context(|| format!("failed to get attributes of {}", file.quote()))?;
-        dest_gid = Some(meta.gid());
-        dest_uid = Some(meta.uid());
+        let gid = meta.gid();
+        let uid = meta.uid();
+        dest_gid = Some(gid);
+        dest_uid = Some(uid);
+        raw_owner = format!(
+            "{}:{}",
+            entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()),
+            entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string())
+        );
     } else {
-        let (u, g) = parse_spec(matches.get_one::<String>(options::ARG_OWNER).unwrap(), ':')?;
+        raw_owner = matches
+            .get_one::<String>(options::ARG_OWNER)
+            .unwrap()
+            .into();
+        let (u, g) = parse_spec(&raw_owner, ':')?;
         dest_uid = u;
         dest_gid = g;
     }
-    Ok((dest_gid, dest_uid, filter))
+    Ok(GidUidOwnerFilter {
+        dest_gid,
+        dest_uid,
+        raw_owner,
+        filter,
+    })
 }
 
 #[uucore::main]

diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs
@@ -182,6 +182,7 @@ pub enum TraverseSymlinks {
 pub struct ChownExecutor {
     pub dest_uid: Option<u32>,
     pub dest_gid: Option<u32>,
+    pub raw_owner: String, // The owner of the file as input by the user in the command line.
     pub traverse_symlinks: TraverseSymlinks,
     pub verbosity: Verbosity,
     pub filter: IfFrom,
@@ -208,7 +209,16 @@ impl ChownExecutor {
         let path = root.as_ref();
         let meta = match self.obtain_meta(path, self.dereference) {
             Some(m) => m,
-            _ => return 1,
+            _ => {
+                if self.verbosity.level == VerbosityLevel::Verbose {
+                    println!(
+                        "failed to change ownership of {} to {}",
+                        path.quote(),
+                        self.raw_owner
+                    );
+                }
+                return 1;
+            }
         };
 
         // Prohibit only if:
@@ -414,7 +424,13 @@ pub mod options {
     pub const ARG_FILES: &str = "FILE";
 }
 
-type GidUidFilterParser = fn(&ArgMatches) -> UResult<(Option<u32>, Option<u32>, IfFrom)>;
+pub struct GidUidOwnerFilter {
+    pub dest_gid: Option<u32>,
+    pub dest_uid: Option<u32>,
+    pub raw_owner: String,
+    pub filter: IfFrom,
+}
+type GidUidFilterOwnerParser = fn(&ArgMatches) -> UResult<GidUidOwnerFilter>;
 
 /// Base implementation for `chgrp` and `chown`.
 ///
@@ -428,7 +444,7 @@ pub fn chown_base(
     mut command: Command,
     args: impl crate::Args,
     add_arg_if_not_reference: &'static str,
-    parse_gid_uid_and_filter: GidUidFilterParser,
+    parse_gid_uid_and_filter: GidUidFilterOwnerParser,
     groups_only: bool,
 ) -> UResult<()> {
     let args: Vec<_> = args.collect();
@@ -511,12 +527,18 @@ pub fn chown_base(
     } else {
         VerbosityLevel::Normal
     };
-    let (dest_gid, dest_uid, filter) = parse_gid_uid_and_filter(&matches)?;
+    let GidUidOwnerFilter {
+        dest_gid,
+        dest_uid,
+        raw_owner,
+        filter,
+    } = parse_gid_uid_and_filter(&matches)?;
 
     let executor = ChownExecutor {
         traverse_symlinks,
         dest_gid,
         dest_uid,
+        raw_owner,
         verbosity: Verbosity {
             groups_only,
             level: verbosity_level,

diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs
@@ -730,15 +730,15 @@ fn test_chown_file_notexisting() {
     let user_name = String::from(result.stdout_str().trim());
     assert!(!user_name.is_empty());
 
-    let _result = scene
+    scene
         .ucmd()
-        .arg(user_name)
+        .arg(&user_name)
         .arg("--verbose")
         .arg("not_existing")
-        .fails();
-
-    // TODO: uncomment once "failed to change ownership of '{}' to {}" added to stdout
-    // result.stderr_contains("retained as");
+        .fails()
+        .stdout_contains(format!(
+            "failed to change ownership of 'not_existing' to {user_name}"
+        ));
     // TODO: uncomment once message changed from "cannot dereference" to "cannot access"
     // result.stderr_contains("cannot access 'not_existing': No such file or directory");
 }