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

rm: add verbose output and trim multiple slashes #1988

Merged
merged 16 commits into from
Apr 5, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
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
13 changes: 12 additions & 1 deletion .github/workflows/CICD.yml
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ jobs:
uses: actions-rs/cargo@v1
with:
command: test
args: --features "feat_os_unix"
args: --features "feat_os_unix" -p uucore -p coreutils
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this

Suggested change
args: --features "feat_os_unix" -p uucore -p coreutils
args: --features "feat_os_unix"

please create a separate PR for this with the rational

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created #2030 which now blocks this pr :)

env:
RUSTFLAGS: '-Awarnings'

Expand Down Expand Up @@ -514,6 +514,17 @@ jobs:
CARGO_UTILITY_LIST_OPTIONS="$(for u in ${UTILITY_LIST}; do echo "-puu_${u}"; done;)"
echo set-output name=UTILITY_LIST::${UTILITY_LIST}
echo ::set-output name=CARGO_UTILITY_LIST_OPTIONS::${CARGO_UTILITY_LIST_OPTIONS}
- name: Test uucore
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

uses: actions-rs/cargo@v1
with:
command: test
args: ${{ steps.vars.outputs.CARGO_FEATURES_OPTION }} --no-fail-fast -p uucore
env:
CARGO_INCREMENTAL: '0'
RUSTC_WRAPPER: ''
RUSTFLAGS: '-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort'
RUSTDOCFLAGS: '-Cpanic=abort'
# RUSTUP_TOOLCHAIN: ${{ steps.vars.outputs.TOOLCHAIN }}
- name: Test
uses: actions-rs/cargo@v1
with:
Expand Down
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,11 @@ If you would prefer to test a select few utilities:
$ cargo test --features "chmod mv tail" --no-default-features
```

Copy link
Contributor

Choose a reason for hiding this comment

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

same

If you also want to test the core utilities:
```bash
$ cargo test -p uucore -p coreutils
```

To debug:
```bash
$ gdb --args target/debug/coreutils ls
Expand Down
3 changes: 2 additions & 1 deletion src/uu/rm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ path = "src/rm.rs"
clap = "2.33"
walkdir = "2.2"
remove_dir_all = "0.5.1"
uucore = { version=">=0.0.7", package="uucore", path="../../uucore" }
uucore = { version=">=0.0.7", package="uucore", path="../../uucore", features=["fs"] }
uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" }

[[bin]]
name = "rm"
path = "src/main.rs"

marvin-bitterlich marked this conversation as resolved.
Show resolved Hide resolved
16 changes: 12 additions & 4 deletions src/uu/rm/src/rm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::collections::VecDeque;
use std::fs;
use std::io::{stderr, stdin, BufRead, Write};
use std::ops::BitOr;
use std::path::Path;
use std::path::{Path, PathBuf};
use walkdir::{DirEntry, WalkDir};

#[derive(Eq, PartialEq, Clone, Copy)]
Expand Down Expand Up @@ -251,7 +251,7 @@ fn handle_dir(path: &Path, options: &Options) -> bool {

let is_root = path.has_root() && path.parent().is_none();
if options.recursive && (!is_root || !options.preserve_root) {
if options.interactive != InteractiveMode::Always {
if options.interactive != InteractiveMode::Always && !options.verbose {
// we need the extra crate because apparently fs::remove_dir_all() does not function
// correctly on Windows
if let Err(e) = remove_dir_all(path) {
Expand Down Expand Up @@ -311,7 +311,7 @@ fn remove_dir(path: &Path, options: &Options) -> bool {
match fs::remove_dir(path) {
Ok(_) => {
if options.verbose {
println!("removed directory '{}'", path.display());
println!("removed directory '{}'", normalize(path).display());
}
}
Err(e) => {
Expand Down Expand Up @@ -349,7 +349,7 @@ fn remove_file(path: &Path, options: &Options) -> bool {
match fs::remove_file(path) {
Ok(_) => {
if options.verbose {
println!("removed '{}'", path.display());
println!("removed '{}'", normalize(path).display());
}
}
Err(e) => {
Expand All @@ -370,6 +370,14 @@ fn prompt_file(path: &Path, is_dir: bool) -> bool {
}
}

fn normalize(path: &Path) -> PathBuf {
// copied from https://github.com/rust-lang/cargo/blob/2e4cfc2b7d43328b207879228a2ca7d427d188bb/src/cargo/util/paths.rs#L65-L90
// both projects are MIT https://github.com/rust-lang/cargo/blob/master/LICENSE-MIT
// for std impl progress see rfc https://github.com/rust-lang/rfcs/issues/2208
// TODO: replace this once that lands
uucore::fs::normalize_path(path)
}

fn prompt(msg: &str) -> bool {
let _ = stderr().write_all(msg.as_bytes());
let _ = stderr().flush();
Expand Down
89 changes: 89 additions & 0 deletions src/uucore/src/lib/features/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,37 @@ pub enum CanonicalizeMode {
Missing,
}

// copied from https://github.com/rust-lang/cargo/blob/2e4cfc2b7d43328b207879228a2ca7d427d188bb/src/cargo/util/paths.rs#L65-L90
// both projects are MIT https://github.com/rust-lang/cargo/blob/master/LICENSE-MIT
// for std impl progress see rfc https://github.com/rust-lang/rfcs/issues/2208
// replace this once that lands
pub fn normalize_path(path: &Path) -> PathBuf {
let mut components = path.components().peekable();
let mut ret = if let Some(c @ Component::Prefix(..)) = components.peek().cloned() {
components.next();
PathBuf::from(c.as_os_str())
} else {
PathBuf::new()
};

for component in components {
match component {
Component::Prefix(..) => unreachable!(),
Component::RootDir => {
ret.push(component.as_os_str());
}
Component::CurDir => {}
Component::ParentDir => {
ret.pop();
}
Component::Normal(c) => {
ret.push(c);
}
}
}
ret
}

fn resolve<P: AsRef<Path>>(original: P) -> IOResult<PathBuf> {
const MAX_LINKS_FOLLOWED: u32 = 255;
let mut followed = 0;
Expand Down Expand Up @@ -266,3 +297,61 @@ pub fn display_permissions_unix(mode: u32) -> String {

result
}

#[cfg(test)]
mod tests {
// Note this useful idiom: importing names from outer (for mod tests) scope.
use super::*;

struct NormalizePathTestCase<'a> {
path: &'a str,
test: &'a str,
}

const NORMALIZE_PATH_TESTS: [NormalizePathTestCase; 8] = [
NormalizePathTestCase {
path: "./foo/bar.txt",
test: "foo/bar.txt",
},
NormalizePathTestCase {
path: "bar/../foo/bar.txt",
test: "foo/bar.txt",
},
NormalizePathTestCase {
path: "foo///bar.txt",
test: "foo/bar.txt",
},
NormalizePathTestCase {
path: "foo///bar",
test: "foo/bar",
},
NormalizePathTestCase {
path: "foo//./bar",
test: "foo/bar",
},
NormalizePathTestCase {
path: "/foo//./bar",
test: "/foo/bar",
},
NormalizePathTestCase {
path: r"C:/you/later/",
test: "C:/you/later",
},
NormalizePathTestCase {
path: "\\networkshare/a//foo//./bar",
test: "\\networkshare/a/foo/bar",
},
];

#[test]
fn test_normalize_path() {
for test in NORMALIZE_PATH_TESTS.iter() {
let path = Path::new(test.path);
marvin-bitterlich marked this conversation as resolved.
Show resolved Hide resolved
let normalized = normalize_path(path);
assert_eq!(
test.test.replace("/", std::path::MAIN_SEPARATOR.to_string().as_str()),
normalized.to_str().expect("Path is not valid utf-8!")
);
}
}
}
29 changes: 29 additions & 0 deletions tests/by-util/test_rm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,3 +263,32 @@ fn test_rm_no_operand() {
ucmd.fails()
.stderr_is("rm: error: missing an argument\nrm: error: for help, try 'rm --help'\n");
}

#[test]
fn test_rm_verbose_slash() {
let (at, mut ucmd) = at_and_ucmd!();
let dir = "test_rm_verbose_slash_directory";
let file_a = &format!("{}/test_rm_verbose_slash_file_a", dir);

at.mkdir(dir);
marvin-bitterlich marked this conversation as resolved.
Show resolved Hide resolved
at.touch(file_a);

let file_a_normalized = &format!(
"{}{}test_rm_verbose_slash_file_a",
dir,
std::path::MAIN_SEPARATOR
);

ucmd.arg("-r")
.arg("-f")
.arg("-v")
.arg(&format!("{}///", dir))
.succeeds()
.stdout_only(format!(
"removed '{}'\nremoved directory '{}'\n",
file_a_normalized, dir
));

assert!(!at.dir_exists(dir));
assert!(!at.file_exists(file_a));
}