Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #4767: chown -v 0 nf isn't showing a message #4768

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/uu/chgrp/src/chgrp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,22 @@ use std::os::unix::fs::MetadataExt;
static 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<(Option<u32>, Option<u32>, String, IfFrom)> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to make a struct for the (Option<u32>, Option<u32>, String, IfFrom)? That would make it a bit easier to read (and make the types shorter).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the String might need to be an OsString if it can contain invalid utf-8.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The raw_owner: String is constructed by either:

  • matches.get_one::<String>( ... ) or
  • format of entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string())

I don't know if these two functions are robust enough to invalid utf-8.
If not, it might imply to refactor at least uucore/*/perms.rs, uu/*/chown.rs, uu/*/chgrp.rs. Then it should be useful to open a dedicated issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored (Option<u32>, Option<u32>, String, IfFrom) to a struct 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 {
Expand All @@ -45,7 +51,7 @@ fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<(Option<u32>, Option<u32>,
}
}
};
Ok((dest_gid, None, IfFrom::All))
Ok((dest_gid, None, raw_group, IfFrom::All))
}

#[uucore::main]
Expand Down
24 changes: 19 additions & 5 deletions src/uu/chown/src/chown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ 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<(Option<u32>, Option<u32>, String, IfFrom)> {
let filter = if let Some(spec) = matches.get_one::<String>(options::FROM) {
match parse_spec(spec, ':')? {
(Some(uid), None) => IfFrom::User(uid),
Expand All @@ -37,17 +39,29 @@ 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((dest_gid, dest_uid, raw_owner, filter))
}

#[uucore::main]
Expand Down
17 changes: 14 additions & 3 deletions src/uucore/src/lib/features/perms.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -207,7 +208,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:
Expand Down Expand Up @@ -412,7 +422,7 @@ pub mod options {
pub const ARG_FILES: &str = "FILE";
}

type GidUidFilterParser = fn(&ArgMatches) -> UResult<(Option<u32>, Option<u32>, IfFrom)>;
type GidUidFilterParser = fn(&ArgMatches) -> UResult<(Option<u32>, Option<u32>, String, IfFrom)>;

/// Base implementation for `chgrp` and `chown`.
///
Expand Down Expand Up @@ -508,12 +518,13 @@ pub fn chown_base(
} else {
VerbosityLevel::Normal
};
let (dest_gid, dest_uid, filter) = parse_gid_uid_and_filter(&matches)?;
let (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,
Expand Down
9 changes: 5 additions & 4 deletions tests/by-util/test_chown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,15 +730,16 @@ fn test_chown_file_notexisting() {
let user_name = String::from(result.stdout_str().trim());
assert!(!user_name.is_empty());

let _result = scene
let result = scene
cakebaker marked this conversation as resolved.
Show resolved Hide resolved
.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");
result.stdout_contains(format!(
"failed to change ownership of 'not_existing' to {user_name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that this message is different from the one that was commented out. Was that just a mistake?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also add this assertion to the rest of the expression:

scene
    .ucmd()
    ...
    .fails()
    .stdout_contains(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual test tests a non existing file with verbose flag:

 chown -v 0 nf

The string "retained as" is printed by the command:

touch f
chown -v --from=42 43 f

These two cases came from the GNU test suite: tests/chown/basic.sh

));
// TODO: uncomment once message changed from "cannot dereference" to "cannot access"
// result.stderr_contains("cannot access 'not_existing': No such file or directory");
}