From 3d0c8ae6e72ac53b50e8de583e7acdf1b59b037c Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Thu, 2 Nov 2023 18:19:56 +0100 Subject: [PATCH 01/15] Expand CONTRIBUTING.md (WIP) --- .../workspace.wordlist.txt | 1 + CONTRIBUTING.md | 267 ++++++++++++++---- 2 files changed, 211 insertions(+), 57 deletions(-) diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index 6d6533bcf5..c3c854a4cd 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -59,6 +59,7 @@ clippy rustc rustfmt rustup +rustdoc # bitor # BitOr trait function bitxor # BitXor trait function diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 695e5ad18e..d9ee528f8f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,52 +1,202 @@ - + # Contributing to coreutils -Contributions are very welcome via Pull Requests. If you don't know where to -start, take a look at the -[`good-first-issues`](https://github.com/uutils/coreutils/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22). -If you have any questions, feel free to ask them in the issues or on -[Discord](https://discord.gg/wQVJbvJ). - -## Best practices - -1. Follow what GNU is doing in terms of options and behavior. It is recommended - to look at the GNU Coreutils manual ([on the - web](https://www.gnu.org/software/coreutils/manual/html_node/index.html), or - locally using `info `). It is more in depth than the man pages and - provides a good description of available features and their implementation - details. -1. If possible, look at the GNU test suite execution in the CI and make the test - work if failing. -1. Use clap for argument management. -1. Make sure that the code coverage is covering all of the cases, including - errors. -1. The code must be clippy-warning-free and rustfmt-compliant. -1. Don't hesitate to move common functions into uucore if they can be reused by - other binaries. -1. Unsafe code should be documented with Safety comments. -1. uutils is original code. It cannot contain code from existing GNU or Unix-like - utilities, nor should it link to or reference GNU libraries. +Hi! Welcome to uutils/coreutils! + +Thanks for wanting to contribute to this project! This document explains +everything you need to know to contribute. Before you start make sure to also +check out these documents: + +- Our community's [CODE_OF_CONDUCT.md](./CODE_OF_CONDUCT.md). +- [DEVELOPMENT.md](./DEVELOPMENT.md) for setting up your development + environment. + +Now follows a very important warning: + +> [!WARNING] +> uutils is original code and cannot contain any code from GNU or +> other implementations. This means that **we cannot accept any changes based on +> the GNU source code**. To make sure that cannot happen, **you cannot link to +> the GNU source code** either. + +Finally, feel free to join our [Discord](https://discord.gg/wQVJbvJ)! + +## Getting Oriented + +uutils is a big project consisting of many parts. Here are the most important +parts for getting started: + +- [`src/uu`](./src/uu/): The code for all utilities +- [`src/uucore`](./src/uucore/): Crate containing all the shared code between + the utilities. +- [`tests/by-util`](./tests/by-util/): The tests for all utilities. +- [`src/bin/coreutils.rs`](./src/bin/coreutils.rs): Code for the multicall + binary. +- [`docs`](./docs/src): the documentation for the website + +Each utility is defined as a separate crate. The structure of each of these +crates is as follows: + +- `Cargo.toml` +- `src/main.rs`: contains only a single macro call +- `src/.rs`: the actual code for the utility +- `.md`: the documentation for the utility + +We have separated repositories for crates that we maintain but also publish for +use by others: + +- [uutils-term-grid](https://github.com/uutils/uutils-term-grid) +- [parse_datetime](https://github.com/uutils/parse_datetime) + +## Design Goals + +todo + +## How to Help + +There are several ways to help and writing code is just one them. Reporting +issues and writing documentation are just as important as writing code. + +### Reporting Issues + +We can't fix bugs we don't know about, so good issues are super helpful! Here +are some tips for writing good issues: + +- If you find a bug, make sure it's still a problem on the `main` branch. +- Search through the existing issues to see whether it has already been + reported. +- Make sure to include all relevant information, such as: + - Which version of uutils did you check? + - Which version of GNU coreutils are you comparing with? + - What platform are you on? +- Provide a way to reliably reproduce the issue. +- Be as specific as possible! + +### Writing Documentation + +There's never enough documentation. If you come across any documentation that +could be improved, feel free to submit a PR for it! + +### Writing Code + +If you want to submit a PR, make sure that you've discussed the solution with +the maintainers beforehand. We want to avoid situations where you put a lot of +work into a fix that we can't merge! If there's no issue for what you're trying +to fix yet, make one _before_ you start working on the PR. + +Generally, we try to follow what GNU is doing in terms of options and behavior. +It is recommended to look at the GNU Coreutils manual +([on the web](https://www.gnu.org/software/coreutils/manual/html_node/index.html), +or locally using `info `). It is more in depth than the man pages and +provides a good description of available features and their implementation +details. But remember, you cannot look at the GNU source code! + +Also remember that we can only merge PRs which pass our test suite, follow +rustfmt, and do not have any warnings from clippy. See +[DEVELOPMENT.md](./DEVELOPMENT.md) for more information. Be sure to also read +about our [Rust style](#our-rust-style). + +## Our Rust Style + +We want uutils to be written in idiomatic Rust, so here are some guidelines to +follow. Some of these are aspirational, meaning that we don't do them correctly +everywhere in the code. If you find violations of the advice below, feel free to +submit a patch! + +### Don't `panic!` + +The coreutils should be very reliable. This means that we should never `panic!`. +Therefore, you should avoid using `.unwrap()` and `panic!`. Sometimes the use of +`unreachable!` can be justified with a comment explaining why that code is +unreachable. + +### Don't `exit` + +We want uutils to be embeddable in other programs. This means that no function +in uutils should exit the program. Doing so would also lead to code with more +confusing control flow. Avoid therefore `std::process::exit` and similar +functions which exit the program early. + +### `unsafe` + +uutils cannot be entirely safe, because we have to call out to `libc` and do +syscalls. However, we still want to limit our use of `unsafe`. We generally only +accept `unsafe` for FFI, with very few exceptions. Note that performance is very +rarely a valid argument for using `unsafe`. + +If you still need to write code with `unsafe`, make sure to read the +[Rustonomicon](https://doc.rust-lang.org/nomicon/intro.html) and annotate the +calls with `// SAFETY:` comments explaining why the use of `unsafe` is sound. + +### Macros + +Macros can be a great tool, but they are also usually hard to understand. They +should be used sparingly. Make sure to explore simpler options before you reach +for a solution involving macros. + +### `str`, `OsStr` & `Path` + +Rust has many string-like types, and sometimes it's hard to choose the right +one. It's tempting to use `str` (and `String`) for everything, but that is not +always the right choice for uutils, because we need to support invalid UTF-8, +just like the GNU coreutils. For example, paths on Linux might not be valid +UTF-8! Whenever we are dealing with paths, we should therefore stick with +`OsStr` and `Path`. Make sure that you only convert to `str`/`String` if you +know that something is always valid UTF-8. If you need more operations on +`OsStr`, you can use the [`bstr`](https://docs.rs/bstr/latest/bstr/) crate. + +### Doc-comments + +We use rustdoc for our documentation, so it's best to follow +[rustdoc's guidelines](https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html#documenting-components). +Make sure that your documentation is not just repeating the name of the +function, but actually giving more useful information. Rustdoc recommends the +following structure: + +``` +[short sentence explaining what it is] + +[more detailed explanation] + +[at least one code example that users can copy/paste to try it] + +[even more advanced explanations if necessary] +``` + +### Other comments + +Comments should be written to _explain_ the code, not to _describe_ the code. +Try to focus on explaining _why_ the code is the way it is. If you feel like you +have to describe the code, that's usually a sign that you could improve the +naming of variables and functions. + +If you edit a piece of code, make sure to update any comments that need to +change as a result. The only thing worse than having no comments is having +outdated comments! + +## Git Etiquette + +todo ## Platforms We take pride in supporting many operating systems and architectures. Any code you contribute must at least compile without warnings for all platforms in the -CI. However, you can use `#[cfg(...)]` attributes to create platform dependent features. +CI. However, you can use `#[cfg(...)]` attributes to create platform dependent +features. **Tip:** For Windows, Microsoft provides some images (VMWare, Hyper-V, VirtualBox and Parallels) for development: -## Setting up your development environment - -To setup your local development environment for this project please follow [DEVELOPMENT.md guide](DEVELOPMENT.md) - -It covers [installation of necessary tools and prerequisites](DEVELOPMENT.md#tools) as well as using those tools to [test your code changes locally](DEVELOPMENT.md#testing) - ## Improving the GNU compatibility -Please make sure you have installed [GNU utils and prerequisites](DEVELOPMENT.md#gnu-utils-and-prerequisites) and can execute commands described in [Comparing with GNU](DEVELOPMENT.md#comparing-with-gnu) section of [DEVELOPMENT.md](DEVELOPMENT.md) +Please make sure you have installed +[GNU utils and prerequisites](DEVELOPMENT.md#gnu-utils-and-prerequisites) and +can execute commands described in +[Comparing with GNU](DEVELOPMENT.md#comparing-with-gnu) section of +[DEVELOPMENT.md](DEVELOPMENT.md) The Python script `./util/remaining-gnu-error.py` shows the list of failing tests in the CI. @@ -103,20 +253,20 @@ Further paragraphs come after blank lines. Furthermore, here are a few examples for a summary line: -* commit for a single utility +- commit for a single utility ``` nohup: cleanup and refactor ``` -* commit for a utility's tests +- commit for a utility's tests ``` tests/rm: test new feature ``` -Beyond changes to an individual utility or its tests, other summary -lines for non-utility modules include: +Beyond changes to an individual utility or its tests, other summary lines for +non-utility modules include: ``` README: add help @@ -136,23 +286,26 @@ gitignore: add temporary files ## Code coverage -To generate code coverage report locally please follow [Code coverage report](DEVELOPMENT.md#code-coverage-report) section of [DEVELOPMENT.md](DEVELOPMENT.md) +To generate code coverage report locally please follow +[Code coverage report](DEVELOPMENT.md#code-coverage-report) section of +[DEVELOPMENT.md](DEVELOPMENT.md) ## Other implementations -The Coreutils have different implementations, with different levels of completions: +The Coreutils have different implementations, with different levels of +completions: -* [GNU's](https://git.savannah.gnu.org/gitweb/?p=coreutils.git) -* [OpenBSD](https://github.com/openbsd/src/tree/master/bin) -* [Busybox](https://github.com/mirror/busybox/tree/master/coreutils) -* [Toybox (Android)](https://github.com/landley/toybox/tree/master/toys/posix) -* [V lang](https://github.com/vlang/coreutils) -* [SerenityOS](https://github.com/SerenityOS/serenity/tree/master/Userland/Utilities) -* [Initial Unix](https://github.com/dspinellis/unix-history-repo) -* [Perl Power Tools](https://metacpan.org/pod/PerlPowerTools) +- [GNU's](https://git.savannah.gnu.org/gitweb/?p=coreutils.git) +- [OpenBSD](https://github.com/openbsd/src/tree/master/bin) +- [Busybox](https://github.com/mirror/busybox/tree/master/coreutils) +- [Toybox (Android)](https://github.com/landley/toybox/tree/master/toys/posix) +- [V lang](https://github.com/vlang/coreutils) +- [SerenityOS](https://github.com/SerenityOS/serenity/tree/master/Userland/Utilities) +- [Initial Unix](https://github.com/dspinellis/unix-history-repo) +- [Perl Power Tools](https://metacpan.org/pod/PerlPowerTools) -However, when reimplementing the tools/options in Rust, don't read their source codes -when they are using reciprocal licenses (ex: GNU GPL, GNU LGPL, etc). +However, when reimplementing the tools/options in Rust, don't read their source +codes when they are using reciprocal licenses (ex: GNU GPL, GNU LGPL, etc). ## Licensing @@ -167,17 +320,17 @@ If you wish to add or change dependencies as part of a contribution to the project, a tool like `cargo-license` can be used to show their license details. The following types of license are acceptable: -* MIT License -* Dual- or tri-license with an MIT License option ("Apache-2.0 or MIT" is a +- MIT License +- Dual- or tri-license with an MIT License option ("Apache-2.0 or MIT" is a popular combination) -* "MIT equivalent" license (2-clause BSD, 3-clause BSD, ISC) -* License less restrictive than the MIT License (CC0 1.0 Universal) -* Apache License version 2.0 +- "MIT equivalent" license (2-clause BSD, 3-clause BSD, ISC) +- License less restrictive than the MIT License (CC0 1.0 Universal) +- Apache License version 2.0 Licenses we will not use: -* An ambiguous license, or no license -* Strongly reciprocal licenses (GNU GPL, GNU LGPL) +- An ambiguous license, or no license +- Strongly reciprocal licenses (GNU GPL, GNU LGPL) If you wish to add a reference but it doesn't meet these requirements, please raise an issue to describe the dependency. From d7b256be7bbbbc91ff95ef3e7805d5733fca7382 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Fri, 3 Nov 2023 11:44:19 +0100 Subject: [PATCH 02/15] CONTRIBUTING.md: write Design Goals section --- CONTRIBUTING.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d9ee528f8f..f59d3b4f87 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -51,7 +51,13 @@ use by others: ## Design Goals -todo +We have the following goals with our development: + +- **Compatible**: The utilities should be a drop-in replacement for the GNU coreutils. +- **Cross-platform**: All utilities should run on as many of the supported platforms as possible. +- **Reliable**: The utilities should never unexpectedly fail. +- **Performant**: Our utilities should be written in fast idiomatic Rust. We aim to match or exceed the performance of the GNU utilities. +- **Well-tested**: We should have a lot of tests to be able to guarantee reliability and compatibility. ## How to Help From e11878e7ba9f6344aa59b21722ba0c042176af6b Mon Sep 17 00:00:00 2001 From: Alexandre Hausen Date: Sun, 5 Nov 2023 20:58:04 -0300 Subject: [PATCH 03/15] du: remove crash! macro --- src/uu/du/src/du.rs | 49 ++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index ae88ed13bd..ddfbc5cc49 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -31,13 +31,11 @@ use std::time::{Duration, UNIX_EPOCH}; use std::{error::Error, fmt::Display}; use uucore::display::{print_verbatim, Quotable}; use uucore::error::FromIo; -use uucore::error::{set_exit_code, UError, UResult}; +use uucore::error::{set_exit_code, UError, UResult, USimpleError}; use uucore::line_ending::LineEnding; use uucore::parse_glob; use uucore::parse_size::{parse_size_u64, ParseSizeError}; -use uucore::{ - crash, format_usage, help_about, help_section, help_usage, show, show_error, show_warning, -}; +use uucore::{format_usage, help_about, help_section, help_usage, show, show_error, show_warning}; #[cfg(windows)] use windows_sys::Win32::Foundation::HANDLE; #[cfg(windows)] @@ -255,22 +253,27 @@ fn get_file_info(path: &Path) -> Option { result } -fn read_block_size(s: Option<&str>) -> u64 { +fn read_block_size(s: Option<&str>) -> UResult { if let Some(s) = s { - parse_size_u64(s) - .unwrap_or_else(|e| crash!(1, "{}", format_error_message(&e, s, options::BLOCK_SIZE))) + match parse_size_u64(s) { + Ok(x) => Ok(x), + Err(e) => Err(USimpleError::new( + 1, + format_error_message(&e, s, options::BLOCK_SIZE), + )), + } } else { for env_var in ["DU_BLOCK_SIZE", "BLOCK_SIZE", "BLOCKSIZE"] { if let Ok(env_size) = env::var(env_var) { if let Ok(v) = parse_size_u64(&env_size) { - return v; + return Ok(v); } } } if env::var("POSIXLY_CORRECT").is_ok() { - 512 + Ok(512) } else { - 1024 + Ok(1024) } } } @@ -574,12 +577,20 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { matches .get_one::(options::BLOCK_SIZE) .map(|s| s.as_str()), - ); + )?; - let threshold = matches.get_one::(options::THRESHOLD).map(|s| { - Threshold::from_str(s) - .unwrap_or_else(|e| crash!(1, "{}", format_error_message(&e, s, options::THRESHOLD))) - }); + let threshold = match matches.get_one::(options::THRESHOLD) { + Some(s) => match Threshold::from_str(s) { + Ok(t) => Some(t), + Err(e) => { + return Err(USimpleError::new( + 1, + format_error_message(&e, s, options::THRESHOLD), + )) + } + }, + None => None, + }; let multiplier: u64 = if matches.get_flag(options::SI) { 1000 @@ -991,13 +1002,9 @@ mod test_du { #[test] fn test_read_block_size() { - let test_data = [ - (Some("1024".to_string()), 1024), - (Some("K".to_string()), 1024), - (None, 1024), - ]; + let test_data = [Some("1024".to_string()), Some("K".to_string()), None]; for it in &test_data { - assert_eq!(read_block_size(it.0.as_deref()), it.1); + assert!(matches!(read_block_size(it.as_deref()), Ok(1024))); } } } From 2571af8ededb52c53097508e4f2e848549f9075c Mon Sep 17 00:00:00 2001 From: Daniel Hofstetter Date: Mon, 6 Nov 2023 10:15:47 +0100 Subject: [PATCH 04/15] du: use blocks to remove some cfgs --- src/uu/du/src/du.rs | 65 ++++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 9139c31c31..53577dcd57 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -137,39 +137,42 @@ impl Stat { }?; #[cfg(not(windows))] - let file_info = FileInfo { - file_id: metadata.ino() as u128, - dev_id: metadata.dev(), - }; - #[cfg(not(windows))] - return Ok(Self { - path: path.to_path_buf(), - is_dir: metadata.is_dir(), - size: if path.is_dir() { 0 } else { metadata.len() }, - blocks: metadata.blocks(), - inodes: 1, - inode: Some(file_info), - created: birth_u64(&metadata), - accessed: metadata.atime() as u64, - modified: metadata.mtime() as u64, - }); + { + let file_info = FileInfo { + file_id: metadata.ino() as u128, + dev_id: metadata.dev(), + }; + + return Ok(Self { + path: path.to_path_buf(), + is_dir: metadata.is_dir(), + size: if path.is_dir() { 0 } else { metadata.len() }, + blocks: metadata.blocks(), + inodes: 1, + inode: Some(file_info), + created: birth_u64(&metadata), + accessed: metadata.atime() as u64, + modified: metadata.mtime() as u64, + }); + } #[cfg(windows)] - let size_on_disk = get_size_on_disk(path); - #[cfg(windows)] - let file_info = get_file_info(path); - #[cfg(windows)] - Ok(Self { - path: path.to_path_buf(), - is_dir: metadata.is_dir(), - size: if path.is_dir() { 0 } else { metadata.len() }, - blocks: size_on_disk / 1024 * 2, - inode: file_info, - inodes: 1, - created: windows_creation_time_to_unix_time(metadata.creation_time()), - accessed: windows_time_to_unix_time(metadata.last_access_time()), - modified: windows_time_to_unix_time(metadata.last_write_time()), - }) + { + let size_on_disk = get_size_on_disk(path); + let file_info = get_file_info(path); + + Ok(Self { + path: path.to_path_buf(), + is_dir: metadata.is_dir(), + size: if path.is_dir() { 0 } else { metadata.len() }, + blocks: size_on_disk / 1024 * 2, + inodes: 1, + inode: file_info, + created: windows_creation_time_to_unix_time(metadata.creation_time()), + accessed: windows_time_to_unix_time(metadata.last_access_time()), + modified: windows_time_to_unix_time(metadata.last_write_time()), + }) + } } } From 993a995f8aa4ad04e46d79f4035dbf3eee02a953 Mon Sep 17 00:00:00 2001 From: Daniel Hofstetter Date: Mon, 6 Nov 2023 10:21:24 +0100 Subject: [PATCH 05/15] du: remove unnecessary return --- src/uu/du/src/du.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 53577dcd57..0d5e34fdeb 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -143,7 +143,7 @@ impl Stat { dev_id: metadata.dev(), }; - return Ok(Self { + Ok(Self { path: path.to_path_buf(), is_dir: metadata.is_dir(), size: if path.is_dir() { 0 } else { metadata.len() }, @@ -153,7 +153,7 @@ impl Stat { created: birth_u64(&metadata), accessed: metadata.atime() as u64, modified: metadata.mtime() as u64, - }); + }) } #[cfg(windows)] From 7afb8461cbcccb54f8285617480c654b1ae160b8 Mon Sep 17 00:00:00 2001 From: Daniel Hofstetter Date: Tue, 7 Nov 2023 10:30:54 +0100 Subject: [PATCH 06/15] du: add -H (alias for --dereference-args) --- src/uu/du/src/du.rs | 1 + tests/by-util/test_du.rs | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 9139c31c31..407d90a546 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -821,6 +821,7 @@ pub fn uu_app() -> Command { .arg( Arg::new(options::DEREFERENCE_ARGS) .short('D') + .visible_short_alias('H') .long(options::DEREFERENCE_ARGS) .help("follow only symlinks that are listed on the command line") .action(ArgAction::SetTrue) diff --git a/tests/by-util/test_du.rs b/tests/by-util/test_du.rs index fdb44ef536..37594217d4 100644 --- a/tests/by-util/test_du.rs +++ b/tests/by-util/test_du.rs @@ -299,11 +299,13 @@ fn test_du_dereference_args() { file2.write_all(b"amaz?ng").unwrap(); at.symlink_dir("subdir", "sublink"); - let result = ts.ucmd().arg("-D").arg("-s").arg("sublink").succeeds(); - let stdout = result.stdout_str(); + for arg in ["-D", "-H", "--dereference-args"] { + let result = ts.ucmd().arg(arg).arg("-s").arg("sublink").succeeds(); + let stdout = result.stdout_str(); - assert!(!stdout.starts_with('0')); - assert!(stdout.contains("sublink")); + assert!(!stdout.starts_with('0')); + assert!(stdout.contains("sublink")); + } // Without the option let result = ts.ucmd().arg("-s").arg("sublink").succeeds(); From 1a457c00aa99ecdf67c15673a93b70d56709f3aa Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Tue, 7 Nov 2023 11:37:17 +0100 Subject: [PATCH 07/15] CONTRIBUTING.md: start on git etiquette --- CONTRIBUTING.md | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f59d3b4f87..ee912d786c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -183,7 +183,27 @@ outdated comments! ## Git Etiquette -todo +To ensure easy collaboration, we have guidelines for using Git and GitHub. + +### Commits + +- Make small and atomic commits. +- Keep a clean history of commits. +- Write informative commit messages. +- Annotate your commit message with the component you're editing. For example: `cp: do not overwrite on with -i` or `uucore: add support for FreeBSD`. +- Do not unnecessarily move items around in the code. This makes the changes much harder to review. If you do need to move things around, do that in a separate commit. + +### PRs + +- Make the titles of PRs descriptive. + - This means describing the problem you solve. For example, do not write `Fix #1234`, but `ls: fix version sort order`. + - You can prefix the title with the utility the PR concerns. +- Keep PRs small and self-contained. A set of small PRs is much more likely to get merged quickly than one large PR. +- Make sure the CI passes (up to intermittently failing tests). +- You know your code best, that's why it's best if you can solve merge conflicts on your branch yourself. + - It's up to you whether you want to use `git merge main` or `git rebase main`. + - Feel free to ask for help with merge conflicts. +- You do not need to ping maintainers to request a review, but it's fine to do so if you don't get a response within a few days. ## Platforms From 6678c17c52b6be1512d568e5b50c80c19018c346 Mon Sep 17 00:00:00 2001 From: Taylor <6225757+tskinn@users.noreply.github.com> Date: Tue, 7 Nov 2023 03:43:58 -0700 Subject: [PATCH 08/15] mktemp: add func to expose functionality (for use in nushell) (#5479) * mktemp: add func to expose functionality * mktemp: cleanup --- src/uu/mktemp/src/mktemp.rs | 58 +++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/src/uu/mktemp/src/mktemp.rs b/src/uu/mktemp/src/mktemp.rs index 77ef5fcbe2..d52351a894 100644 --- a/src/uu/mktemp/src/mktemp.rs +++ b/src/uu/mktemp/src/mktemp.rs @@ -115,29 +115,30 @@ impl Display for MkTempError { /// This provides a layer of indirection between the application logic /// and the argument parsing library `clap`, allowing each to vary /// independently. -struct Options { +#[derive(Clone)] +pub struct Options { /// Whether to create a temporary directory instead of a file. - directory: bool, + pub directory: bool, /// Whether to just print the name of a file that would have been created. - dry_run: bool, + pub dry_run: bool, /// Whether to suppress file creation error messages. - quiet: bool, + pub quiet: bool, /// The directory in which to create the temporary file. /// /// If `None`, the file will be created in the current directory. - tmpdir: Option, + pub tmpdir: Option, /// The suffix to append to the temporary file, if any. - suffix: Option, + pub suffix: Option, /// Whether to treat the template argument as a single file path component. - treat_as_template: bool, + pub treat_as_template: bool, /// The template to use for the name of the temporary file. - template: String, + pub template: String, } impl Options { @@ -192,7 +193,7 @@ impl Options { /// `num_rand_chars`. struct Params { /// The directory that will contain the temporary file. - directory: String, + directory: PathBuf, /// The (non-random) prefix of the temporary file. prefix: String, @@ -297,7 +298,7 @@ impl Params { let num_rand_chars = j - i; Ok(Self { - directory, + directory: directory.into(), prefix, num_rand_chars, suffix, @@ -357,12 +358,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { exec(&tmpdir, &prefix, rand, &suffix, make_dir) }; - if suppress_file_err { + let res = if suppress_file_err { // Mapping all UErrors to ExitCodes prevents the errors from being printed res.map_err(|e| e.code().into()) } else { res - } + }; + println_verbatim(res?).map_err_context(|| "failed to print directory name".to_owned()) } pub fn uu_app() -> Command { @@ -441,7 +443,7 @@ pub fn uu_app() -> Command { .arg(Arg::new(ARG_TEMPLATE).num_args(..=1)) } -pub fn dry_exec(tmpdir: &str, prefix: &str, rand: usize, suffix: &str) -> UResult<()> { +fn dry_exec(tmpdir: &Path, prefix: &str, rand: usize, suffix: &str) -> UResult { let len = prefix.len() + suffix.len() + rand; let mut buf = Vec::with_capacity(len); buf.extend(prefix.as_bytes()); @@ -462,7 +464,7 @@ pub fn dry_exec(tmpdir: &str, prefix: &str, rand: usize, suffix: &str) -> UResul // We guarantee utf8. let buf = String::from_utf8(buf).unwrap(); let tmpdir = Path::new(tmpdir).join(buf); - println_verbatim(tmpdir).map_err_context(|| "failed to print directory name".to_owned()) + Ok(tmpdir) } /// Create a temporary directory with the given parameters. @@ -476,7 +478,7 @@ pub fn dry_exec(tmpdir: &str, prefix: &str, rand: usize, suffix: &str) -> UResul /// /// If the temporary directory could not be written to disk or if the /// given directory `dir` does not exist. -fn make_temp_dir(dir: &str, prefix: &str, rand: usize, suffix: &str) -> UResult { +fn make_temp_dir(dir: &Path, prefix: &str, rand: usize, suffix: &str) -> UResult { let mut builder = Builder::new(); builder.prefix(prefix).rand_bytes(rand).suffix(suffix); match builder.tempdir_in(dir) { @@ -508,7 +510,7 @@ fn make_temp_dir(dir: &str, prefix: &str, rand: usize, suffix: &str) -> UResult< /// /// If the file could not be written to disk or if the directory does /// not exist. -fn make_temp_file(dir: &str, prefix: &str, rand: usize, suffix: &str) -> UResult { +fn make_temp_file(dir: &Path, prefix: &str, rand: usize, suffix: &str) -> UResult { let mut builder = Builder::new(); builder.prefix(prefix).rand_bytes(rand).suffix(suffix); match builder.tempfile_in(dir) { @@ -527,7 +529,7 @@ fn make_temp_file(dir: &str, prefix: &str, rand: usize, suffix: &str) -> UResult } } -fn exec(dir: &str, prefix: &str, rand: usize, suffix: &str, make_dir: bool) -> UResult<()> { +fn exec(dir: &Path, prefix: &str, rand: usize, suffix: &str, make_dir: bool) -> UResult { let path = if make_dir { make_temp_dir(dir, prefix, rand, suffix)? } else { @@ -546,7 +548,27 @@ fn exec(dir: &str, prefix: &str, rand: usize, suffix: &str, make_dir: bool) -> U // relative path. let path = Path::new(dir).join(filename); - println_verbatim(path).map_err_context(|| "failed to print directory name".to_owned()) + Ok(path) +} + +/// Create a temporary file or directory +/// +/// Behavior is determined by the `options` parameter, see [`Options`] for details. +pub fn mktemp(options: &Options) -> UResult { + // Parse file path parameters from the command-line options. + let Params { + directory: tmpdir, + prefix, + num_rand_chars: rand, + suffix, + } = Params::from(options.clone())?; + + // Create the temporary file or directory, or simulate creating it. + if options.dry_run { + dry_exec(&tmpdir, &prefix, rand, &suffix) + } else { + exec(&tmpdir, &prefix, rand, &suffix, options.directory) + } } #[cfg(test)] From f5709ded4743f96a852baa8134e09637b9619987 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Tue, 7 Nov 2023 12:08:44 +0100 Subject: [PATCH 09/15] CONTRIBUTING.md: move and shorten commit message advice --- CONTRIBUTING.md | 135 ++++++++++++++++++++---------------------------- 1 file changed, 56 insertions(+), 79 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ee912d786c..aea215aece 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,8 +14,7 @@ check out these documents: Now follows a very important warning: -> [!WARNING] -> uutils is original code and cannot contain any code from GNU or +> [!WARNING] uutils is original code and cannot contain any code from GNU or > other implementations. This means that **we cannot accept any changes based on > the GNU source code**. To make sure that cannot happen, **you cannot link to > the GNU source code** either. @@ -53,15 +52,19 @@ use by others: We have the following goals with our development: -- **Compatible**: The utilities should be a drop-in replacement for the GNU coreutils. -- **Cross-platform**: All utilities should run on as many of the supported platforms as possible. +- **Compatible**: The utilities should be a drop-in replacement for the GNU + coreutils. +- **Cross-platform**: All utilities should run on as many of the supported + platforms as possible. - **Reliable**: The utilities should never unexpectedly fail. -- **Performant**: Our utilities should be written in fast idiomatic Rust. We aim to match or exceed the performance of the GNU utilities. -- **Well-tested**: We should have a lot of tests to be able to guarantee reliability and compatibility. +- **Performant**: Our utilities should be written in fast idiomatic Rust. We aim + to match or exceed the performance of the GNU utilities. +- **Well-tested**: We should have a lot of tests to be able to guarantee + reliability and compatibility. ## How to Help -There are several ways to help and writing code is just one them. Reporting +There are several ways to help and writing code is just one of them. Reporting issues and writing documentation are just as important as writing code. ### Reporting Issues @@ -92,7 +95,7 @@ work into a fix that we can't merge! If there's no issue for what you're trying to fix yet, make one _before_ you start working on the PR. Generally, we try to follow what GNU is doing in terms of options and behavior. -It is recommended to look at the GNU Coreutils manual +It is recommended to look at the GNU coreutils manual ([on the web](https://www.gnu.org/software/coreutils/manual/html_node/index.html), or locally using `info `). It is more in depth than the man pages and provides a good description of available features and their implementation @@ -190,20 +193,58 @@ To ensure easy collaboration, we have guidelines for using Git and GitHub. - Make small and atomic commits. - Keep a clean history of commits. - Write informative commit messages. -- Annotate your commit message with the component you're editing. For example: `cp: do not overwrite on with -i` or `uucore: add support for FreeBSD`. -- Do not unnecessarily move items around in the code. This makes the changes much harder to review. If you do need to move things around, do that in a separate commit. +- Annotate your commit message with the component you're editing. For example: + `cp: do not overwrite on with -i` or `uucore: add support for FreeBSD`. +- Do not unnecessarily move items around in the code. This makes the changes + much harder to review. If you do need to move things around, do that in a + separate commit. + +### Commit messages + +You can read this section in the Git book to learn how to write good commit +messages: https://git-scm.com/book/ch5-2.html. + +In addition, here are a few examples for a summary line when committing to +uutils: + +- commit for a single utility + +``` +nohup: cleanup and refactor +``` + +- commit for a utility's tests + +``` +tests/rm: test new feature +``` + +Beyond changes to an individual utility or its tests, other summary lines for +non-utility modules include: + +``` +README: add help +uucore: add new modules +uutils: add new utility +gitignore: add temporary files +``` ### PRs - Make the titles of PRs descriptive. - - This means describing the problem you solve. For example, do not write `Fix #1234`, but `ls: fix version sort order`. + - This means describing the problem you solve. For example, do not write + `Fix #1234`, but `ls: fix version sort order`. - You can prefix the title with the utility the PR concerns. -- Keep PRs small and self-contained. A set of small PRs is much more likely to get merged quickly than one large PR. +- Keep PRs small and self-contained. A set of small PRs is much more likely to + get merged quickly than one large PR. - Make sure the CI passes (up to intermittently failing tests). -- You know your code best, that's why it's best if you can solve merge conflicts on your branch yourself. - - It's up to you whether you want to use `git merge main` or `git rebase main`. +- You know your code best, that's why it's best if you can solve merge conflicts + on your branch yourself. + - It's up to you whether you want to use `git merge main` or + `git rebase main`. - Feel free to ask for help with merge conflicts. -- You do not need to ping maintainers to request a review, but it's fine to do so if you don't get a response within a few days. +- You do not need to ping maintainers to request a review, but it's fine to do + so if you don't get a response within a few days. ## Platforms @@ -246,70 +287,6 @@ To improve the GNU compatibility, the following process is recommended: 1. Start to modify the Rust implementation to match the expected behavior 1. Add a test to make sure that we don't regress (our test suite is super quick) -## Commit messages - -To help the project maintainers review pull requests from contributors across -numerous utilities, the team has settled on conventions for commit messages. - -From : - -``` -Capitalized, short (50 chars or less) summary - -More detailed explanatory text, if necessary. Wrap it to about 72 -characters or so. In some contexts, the first line is treated as the -subject of an email and the rest of the text as the body. The blank -line separating the summary from the body is critical (unless you omit -the body entirely); tools like rebase will confuse you if you run the -two together. - -Write your commit message in the imperative: "Fix bug" and not "Fixed bug" -or "Fixes bug." This convention matches up with commit messages generated -by commands like git merge and git revert. - -Further paragraphs come after blank lines. - - - Bullet points are okay, too - - - Typically a hyphen or asterisk is used for the bullet, followed by a - single space, with blank lines in between, but conventions vary here - - - Use a hanging indent -``` - -Furthermore, here are a few examples for a summary line: - -- commit for a single utility - -``` -nohup: cleanup and refactor -``` - -- commit for a utility's tests - -``` -tests/rm: test new feature -``` - -Beyond changes to an individual utility or its tests, other summary lines for -non-utility modules include: - -``` -README: add help -``` - -``` -uucore: add new modules -``` - -``` -uutils: add new utility -``` - -``` -gitignore: add temporary files -``` - ## Code coverage To generate code coverage report locally please follow From 761213f1d2ad1784a00e9145817bd7c4919d92dc Mon Sep 17 00:00:00 2001 From: Daniel Hofstetter Date: Tue, 7 Nov 2023 15:47:04 +0100 Subject: [PATCH 10/15] cp: make test_closes_file_descriptors Linux-only --- tests/by-util/test_cp.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 311558d387..c8761fab8f 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -24,8 +24,6 @@ use std::path::PathBuf; #[cfg(any(target_os = "linux", target_os = "android"))] use filetime::FileTime; -#[cfg(any(target_os = "linux", target_os = "android"))] -use rlimit::Resource; #[cfg(target_os = "linux")] use std::ffi::OsString; #[cfg(any(target_os = "linux", target_os = "android"))] @@ -2120,10 +2118,11 @@ fn test_cp_reflink_insufficient_permission() { .stderr_only("cp: 'unreadable' -> 'existing_file.txt': Permission denied (os error 13)\n"); } -#[cfg(any(target_os = "linux", target_os = "android"))] +#[cfg(target_os = "linux")] #[test] fn test_closes_file_descriptors() { use procfs::process::Process; + use rlimit::Resource; let me = Process::myself().unwrap(); // The test suite runs in parallel, we have pipe, sockets @@ -2133,7 +2132,6 @@ fn test_closes_file_descriptors() { let limit_fd: u64 = number_file_already_opened + 9; // For debugging purposes: - #[cfg(not(target_os = "android"))] for f in me.fd().unwrap() { let fd = f.unwrap(); println!("{:?} {:?}", fd, fd.mode()); From a26e3db56e4778a4dd6058b14fad6912324a302b Mon Sep 17 00:00:00 2001 From: Daniel Hofstetter Date: Wed, 8 Nov 2023 10:00:26 +0100 Subject: [PATCH 11/15] CONTRIBUTING.md: spell-checker:ignore "rustdoc's" --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aea215aece..255ed2c53e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,4 +1,4 @@ - + # Contributing to coreutils From 4d40555cd58dbd82f908cbd20ea57d617fe6e7b3 Mon Sep 17 00:00:00 2001 From: Alexandre Hausen Date: Wed, 8 Nov 2023 20:32:03 -0300 Subject: [PATCH 12/15] fixup! du: remove crash! macro --- src/uu/du/src/du.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index ddfbc5cc49..3a8ac4b145 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -255,13 +255,8 @@ fn get_file_info(path: &Path) -> Option { fn read_block_size(s: Option<&str>) -> UResult { if let Some(s) = s { - match parse_size_u64(s) { - Ok(x) => Ok(x), - Err(e) => Err(USimpleError::new( - 1, - format_error_message(&e, s, options::BLOCK_SIZE), - )), - } + parse_size_u64(s) + .map_err(|e| USimpleError::new(1, format_error_message(&e, s, options::BLOCK_SIZE))) } else { for env_var in ["DU_BLOCK_SIZE", "BLOCK_SIZE", "BLOCKSIZE"] { if let Ok(env_size) = env::var(env_var) { From 7279bc1741c0ee7678a4a66d26392a1ec7dcfed9 Mon Sep 17 00:00:00 2001 From: Zhuoxun Yang Date: Thu, 9 Nov 2023 10:16:53 +0800 Subject: [PATCH 13/15] printf.md: support %q --- src/uu/printf/printf.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/uu/printf/printf.md b/src/uu/printf/printf.md index 60b50354c6..9ce2957701 100644 --- a/src/uu/printf/printf.md +++ b/src/uu/printf/printf.md @@ -78,6 +78,9 @@ Fields second parameter is min-width, integer output below that width is padded with leading zeroes +* `%q`: ARGUMENT is printed in a format that can be reused as shell input, escaping non-printable + characters with the proposed POSIX $'' syntax. + * `%f` or `%F`: decimal floating point value * `%e` or `%E`: scientific notation floating point value * `%g` or `%G`: shorter of specially interpreted decimal or SciNote floating point value. @@ -181,6 +184,11 @@ All string fields have a 'max width' parameter still be interpreted and not throw a warning, you will have problems if you use this for a literal whose code begins with zero, as it will be viewed as in `\\0NNN` form.) +* `%q`: escaped string - the string in a format that can be reused as input by most shells. + Non-printable characters are escaped with the POSIX proposed ‘$''’ syntax, + and shell meta-characters are quoted appropriately. + This is an equivalent format to ls --quoting=shell-escape output. + #### CHAR SUBSTITUTIONS The character field does not have a secondary parameter. From e3ec12233b04f2846769a585aa0f73b64af08833 Mon Sep 17 00:00:00 2001 From: Zhuoxun Yang Date: Thu, 9 Nov 2023 10:17:44 +0800 Subject: [PATCH 14/15] printf: support %q --- src/uucore/src/lib/features/tokenize/sub.rs | 22 ++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/uucore/src/lib/features/tokenize/sub.rs b/src/uucore/src/lib/features/tokenize/sub.rs index c65a37a689..0ae966fc33 100644 --- a/src/uucore/src/lib/features/tokenize/sub.rs +++ b/src/uucore/src/lib/features/tokenize/sub.rs @@ -10,6 +10,7 @@ //! Subs which have numeric field chars make use of the num_format //! submodule use crate::error::{UError, UResult}; +use crate::quoting_style::{escape_name, QuotingStyle}; use itertools::{put_back_n, PutBackN}; use std::error::Error; use std::fmt::Display; @@ -91,7 +92,7 @@ impl Sub { // for more dry printing, field characters are grouped // in initialization of token. let field_type = match field_char { - 's' | 'b' => FieldType::Strf, + 's' | 'b' | 'q' => FieldType::Strf, 'd' | 'i' | 'u' | 'o' | 'x' | 'X' => FieldType::Intf, 'f' | 'F' => FieldType::Floatf, 'a' | 'A' => FieldType::CninetyNineHexFloatf, @@ -189,7 +190,7 @@ impl SubParser { let mut legal_fields = [ // 'a', 'A', //c99 hex float implementation not yet complete - 'b', 'c', 'd', 'e', 'E', 'f', 'F', 'g', 'G', 'i', 'o', 's', 'u', 'x', 'X', + 'b', 'c', 'd', 'e', 'E', 'f', 'F', 'g', 'G', 'i', 'o', 'q', 's', 'u', 'x', 'X', ]; let mut specifiers = ['h', 'j', 'l', 'L', 't', 'z']; legal_fields.sort_unstable(); @@ -260,7 +261,6 @@ impl SubParser { } x if legal_fields.binary_search(&x).is_ok() => { self.field_char = Some(ch); - self.text_so_far.push(ch); break; } x if specifiers.binary_search(&x).is_ok() => { @@ -331,7 +331,7 @@ impl SubParser { if (field_char == 's' && self.min_width_tmp == Some(String::from("0"))) || (field_char == 'c' && (self.min_width_tmp == Some(String::from("0")) || self.past_decimal)) - || (field_char == 'b' + || ((field_char == 'b' || field_char == 'q') && (self.min_width_tmp.is_some() || self.past_decimal || self.second_field_tmp.is_some())) @@ -391,6 +391,7 @@ impl Sub { // if %s just return arg // if %b use UnescapedText module's unescape-fn // if %c return first char of arg + // if %q return arg which non-printable characters are escaped FieldType::Strf | FieldType::Charf => { match pf_arg { Some(arg_string) => { @@ -404,11 +405,18 @@ impl Sub { UnescapedText::from_it_core(writer, &mut a_it, true); None } - // for 'c': get iter of string vals, + 'q' => Some(escape_name( + arg_string.as_ref(), + &QuotingStyle::Shell { + escape: true, + always_quote: false, + show_control: false, + }, + )), // get opt of first val // and map it to opt - /* 'c' | */ - _ => arg_string.chars().next().map(|x| x.to_string()), + 'c' => arg_string.chars().next().map(|x| x.to_string()), + _ => unreachable!(), } } None => None, From fb414ed9179a985de0e545182395a3bd6effbb02 Mon Sep 17 00:00:00 2001 From: Zhuoxun Yang Date: Thu, 9 Nov 2023 10:18:27 +0800 Subject: [PATCH 15/15] tests/printf: support %q --- tests/by-util/test_printf.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/by-util/test_printf.rs b/tests/by-util/test_printf.rs index d7ba5679ec..a297dbf683 100644 --- a/tests/by-util/test_printf.rs +++ b/tests/by-util/test_printf.rs @@ -112,6 +112,15 @@ fn sub_b_string_handle_escapes() { .stdout_only("hello \tworld"); } +#[test] +fn sub_b_string_validate_field_params() { + new_ucmd!() + .args(&["hello %7b", "world"]) + .run() + .stdout_is("hello ") + .stderr_is("printf: %7b: invalid conversion specification\n"); +} + #[test] fn sub_b_string_ignore_subs() { new_ucmd!() @@ -120,6 +129,31 @@ fn sub_b_string_ignore_subs() { .stdout_only("hello world %% %i"); } +#[test] +fn sub_q_string_non_printable() { + new_ucmd!() + .args(&["non-printable: %q", "\"$test\""]) + .succeeds() + .stdout_only("non-printable: '\"$test\"'"); +} + +#[test] +fn sub_q_string_validate_field_params() { + new_ucmd!() + .args(&["hello %7q", "world"]) + .run() + .stdout_is("hello ") + .stderr_is("printf: %7q: invalid conversion specification\n"); +} + +#[test] +fn sub_q_string_special_non_printable() { + new_ucmd!() + .args(&["non-printable: %q", "test~"]) + .succeeds() + .stdout_only("non-printable: test~"); +} + #[test] fn sub_char() { new_ucmd!()